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

[Justin Ruggles]

Justin Ruggles wrote:
> Michael Niedermayer wrote:
>> 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?
> 
> User sets NULL or too small extradata. Our encoder would never do this.
> And stream copy should fail at write_header() in such cases (although it
> does not currently). But if the user does something stupid, this
> prevents trying to write to the extradata if it's NULL or too small. We

sorry, i meant "trying to read from extradata"

> cannot rely on the extradata not changing between write_header() and
> write_trailer() correct?