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

[Justin Ruggles]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac_muxer_extradata_2.diff
Type: text/x-diff
Size: 2331 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090212/275d56f2/attachment.diff>