[FFmpeg-devel] [PATCH] fix raw FLAC muxer extradata handling

[Michael Niedermayer]

On Thu, Feb 12, 2009 at 10:07:52PM -0500, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Thu, Feb 12, 2009 at 08:06:25PM -0500, Justin Ruggles wrote:
> >> Michael Niedermayer wrote:
> >>> On Thu, Feb 12, 2009 at 07:26:33PM -0500, Justin Ruggles wrote:
> >>>> Hi,
> >>>>
> >>>> This patch makes it so the raw FLAC muxer supports either full header or
> >>>> STREAMINFO-only extradata.  It fixes support for converting FLAC-in-MKV
> >>>> to raw FLAC using -acodec copy.
> >>>>
> >>>> -Justin
> >>>>
> >>>> Index: libavformat/raw.c
> >>>> ===================================================================
> >>>> --- libavformat/raw.c	(revision 17188)
> >>>> +++ libavformat/raw.c	(working copy)
> >>>> @@ -24,6 +24,7 @@
> >>>>  #include "libavcodec/ac3_parser.h"
> >>>>  #include "libavcodec/bitstream.h"
> >>>>  #include "libavcodec/bytestream.h"
> >>>> +#include "libavcodec/flac.h"
> >>>>  #include "avformat.h"
> >>>>  #include "raw.h"
> >>>>  #include "id3v2.h"
> >>>> @@ -37,8 +38,12 @@
> >>>>      };
> >>>>      uint8_t *streaminfo = s->streams[0]->codec->extradata;
> >>>>      int len = s->streams[0]->codec->extradata_size;
> >>>> -    if(streaminfo != NULL && len > 0) {
> >>>> -        put_buffer(s->pb, header, 8);
> >>>> +    if (streaminfo != NULL && len >= FLAC_STREAMINFO_SIZE) {
> >>>> +        if (len == FLAC_STREAMINFO_SIZE) {
> >>>> +            put_buffer(s->pb, header, 8);
> >>>> +        } else if (AV_RL32(streaminfo) != MKTAG('f','L','a','C') || len < 8+FLAC_STREAMINFO_SIZE) {
> >>>> +            return 0;
> >>> for which case is this?
> >> Case when extradata does not contain only STREAMINFO, nor a valid header.
> > 
> > I didnt mean that ..
> > my question was more in what case, as in "a file from foogar" ...
> > also considering this hypothetical case, what would the code do?
> > imean not (return 0) but rather
> > create valid flac, create invalid file, print error?
> 
> point taken. this would silently create invalid files for incorrectly
> formatted or wrong length extradata, whether from the user or from an
> invalid input file.
> 
> >>> also the whole does not look particularely robust, i mean an extra byte at the
> >>> end will make it fail silently, it would be different if there was an error
> >>> message ...
> >> I see your point.  An alternative solution would be to only check for
> >> "fLaC" and not extradata_size to determine the format of the extradata.
> >>  The size checks could be done in each branch to just ensure minimum
> >> sizes. I can also add an error message and error return value for
> >> invalid extradata.  If that sounds better I can submit a new patch shortly.
> > 
> > The key goal i have is
> > * do not generate invalid files, at least not without a huge warning
> > * when it fails it should do so with noise to asist bug analysis and debuging
> > * simple, clean, fast, readable (you know, the big goals of every programmer ...)
> 
> new patch attached.
> 
> For flac_write_header(), it fails with an error message and returns an
> error value when there is no extradata or if it is too short since such
> cases would always create an invalid file.  It allows for the
> hypothetical case you gave where the extradata is STREAMINFO plus some
> extra unneeded bytes, but prints a warning message and still creates a
> valid file if the first 34 bytes are valid.
> 
> For flac_write_trailer(), no error is returned for too short or missing
> metadata since rewriting the STREAMINFO is not necessary for a valid
> FLAC file. A warning message is printed in such cases though.
> 
> -Justin
> 

> Index: libavformat/raw.c
> ===================================================================
> --- libavformat/raw.c	(revision 17188)
> +++ libavformat/raw.c	(working copy)
> @@ -24,6 +24,7 @@
>  #include "libavcodec/ac3_parser.h"
>  #include "libavcodec/bitstream.h"
>  #include "libavcodec/bytestream.h"
> +#include "libavcodec/flac.h"
>  #include "avformat.h"
>  #include "raw.h"
>  #include "id3v2.h"
> @@ -37,9 +38,23 @@
>      };
>      uint8_t *streaminfo = s->streams[0]->codec->extradata;
>      int len = s->streams[0]->codec->extradata_size;
> -    if(streaminfo != NULL && len > 0) {
> +    if (streaminfo != NULL && len >= FLAC_STREAMINFO_SIZE) {
> +        if (AV_RL32(streaminfo) != MKTAG('f','L','a','C')) {
> +            /* extradata contains STREAMINFO only */
> +            if (len != FLAC_STREAMINFO_SIZE) {
> +                av_log(s, AV_LOG_WARNING, "extradata contains %d bytes too many.\n",
> +                       FLAC_STREAMINFO_SIZE-len);
> +            }
>          put_buffer(s->pb, header, 8);
> +            len = FLAC_STREAMINFO_SIZE;
> +        } else if (len < 8+FLAC_STREAMINFO_SIZE) {
> +            av_log(s, AV_LOG_ERROR, "extradata too small.\n");
> +            return -1;
> +        }
>          put_buffer(s->pb, streaminfo, len);
> +    } else {
> +        av_log(s, AV_LOG_ERROR, "extradata not found or too small.\n");
> +        return -1;
>      }
>      return 0;
>  }

> @@ -51,12 +66,23 @@
>      int len = s->streams[0]->codec->extradata_size;
>      int64_t file_size;
>  
> -    if (streaminfo && len > 0 && !url_is_streamed(s->pb)) {
> +    if (streaminfo && len >= FLAC_STREAMINFO_SIZE && !url_is_streamed(s->pb)) {

how can the first 2 conditions be false here?
also i dont see how the code here really can be considered a step toward
a solution.
flac can be stored in many containers, we cannot duplicate these checks in
every. ogg_build_flac_headers() being just one spot where this stuff is
needed as well

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090215/3de077c3/attachment.pgp>