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

[Michael Niedermayer]

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?


> 
> > 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 ...)

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

No snowflake in an avalanche ever feels responsible. -- 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/20090213/45fddda7/attachment.pgp>