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

[Justin Ruggles]

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?

> 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,
                               int extradata_size,
                               int *format,
                               uint8_t *streaminfo_start);


This could be used by the Ogg, Matroska, and FLAC muxers, and by the
FLAC decoder at codec init.

-Justin