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

Justin Ruggles justin.ruggles
Sat Feb 21 02:57:09 CET 2009


Michael Niedermayer wrote:
> On Sun, Feb 15, 2009 at 07:41:26PM -0500, 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
>> cannot rely on the extradata not changing between write_header() and
>> write_trailer() correct?
> 
> extradata should generally not change, there are some rare cases like
> updating some frame count or so where it might because it cant be
> done the other way around easily but in general no extradata should
> not change and especially not significatly
> 
> 
>>> 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
>> I did not notice before you brought it up, but ogg_build_flac_headers()
>> needs to be changed as well, as it currently does not support full
>> header extradata.  Matroska supports either type, but only checks the
>> extradata_size, not the "fLaC" signature.  AFAIK, the rest that support
>> FLAC just write all the extradata as-is.
>>
>> What do you think about a shared function such as:
>>
>> enum {
>>     FLAC_EXTRADATA_FORMAT_STREAMINFO  = 0,
>>     FLAC_EXTRADATA_FORMAT_FULL_HEADER = 1
>> }
>> /**
>>  * Validate FLAC extradata.
>>  * @param[out] format format of the extradata.
>>  * @param[out] streaminfo_start pointer in extradata to
>>  *                              start of STREAMINFO.
>>  * @return zero for success, non-zero for error.
>>  */
>> int ff_flac_validate_extradata(uint8_t *extradata,
> 
> ff_is_extradata_valid()
> makes the return meaning obvious and thus means no need to read the doxy
> 
> and i like the idea

Patch set attached.

[PATCH 1/5] Use a shared function to validate FLAC extradata.
[PATCH 2/5] Add support for full header extradata to raw FLAC muxer.
[PATCH 3/5] cosmetics: add a comment in flac_write_header().
[PATCH 4/5] Share the function to write a raw FLAC header and use in the
Matroska muxer.
[PATCH 5/5] cosmetics: line wrap and indentation

Thanks,
Justin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-a-shared-function-to-validate-FLAC-extradata.patch
Type: text/x-diff
Size: 9013 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/9310f008/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-support-for-full-header-extradata-to-raw-FLAC-mu.patch
Type: text/x-diff
Size: 782 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/9310f008/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-cosmetics-add-a-comment-in-flac_write_header.patch
Type: text/x-diff
Size: 417 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/9310f008/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Share-the-function-to-write-a-raw-FLAC-header-and-us.patch
Type: text/x-diff
Size: 3369 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/9310f008/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-cosmetics-line-wrap-and-indentation.patch
Type: text/x-diff
Size: 722 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/9310f008/attachment-0004.patch>



More information about the ffmpeg-devel mailing list