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

[Justin Ruggles]

Michael Niedermayer wrote:
> On Sat, Feb 21, 2009 at 04:27:54PM -0500, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Feb 20, 2009 at 08:57:09PM -0500, Justin Ruggles wrote:
>>>> 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
>>>>
>> New patches attached. I did not attach the cosmetic patches this time or
>> PATCH 2/5 which was already approved.
>>
>>>> diff --git a/libavcodec/flac.h b/libavcodec/flac.h
>>>> index 9a4f820..3dabf07 100644
>>>> --- a/libavcodec/flac.h
>>>> +++ b/libavcodec/flac.h
>>>> @@ -42,6 +42,11 @@ enum {
>>>>      FLAC_METADATA_TYPE_INVALID = 127
>>>>  };
>>>>  
>>>> +enum {
>>>> +    FLAC_EXTRADATA_FORMAT_STREAMINFO  = 0,
>>>> +    FLAC_EXTRADATA_FORMAT_FULL_HEADER = 1
>>>> +};
>>>> +
>>>>  /**
>>>>   * Data needed from the Streaminfo header for use by the raw FLAC demuxer
>>>>   * and/or the FLAC decoder.
>>>
>>>> @@ -68,4 +73,14 @@ typedef struct FLACStreaminfo {
>>>>  void ff_flac_parse_streaminfo(AVCodecContext *avctx, struct FLACStreaminfo *s,
>>>>                                const uint8_t *buffer);
>>>>  
>>>> +/**
>>>> + * Validate the FLAC extradata.
>>>> + * @param[in]  avctx codec context containing the extradata.
>>>> + * @param[out] format extradata format.
>>>> + * @param[out] streaminfo_start pointer to start of 34-byte STREAMINFO data.
>>>> + * @return 1 if valid, 0 if not valid.
>>>> + */
>>>> +int ff_flac_is_extradata_valid(AVCodecContext *avctx, int *format,
>>>                                                          ^^^
>>> enum
>> fixed.
>>
>>>  [...]
>>>> diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
>>> [...]
>>> not maintained by me
>> Baptiste?
>>
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index 25f20fc..f0c8f57 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>> not maintained by me
>> Aurel, I updated the last patch to remove the inclusion of flac.h and
>> add dependencies on raw.o to matroska_muxer and matroska_audio_muxer.
>>
>> I moved ff_flac_write_header() to inside of CONFIG_FLAC_MUXER so that
>> all the other (de)muxers in raw.o don't depend on libavcodec/flacdec.o.
>>  Due to that change, I also updated configure for matroska_muxer and
>> matroska_audio_muxer to depend on flac_muxer.
>>
>> Thanks,
>> Justin
>>
>>
> 
> [...]
>> --- a/libavformat/raw.c
>> +++ b/libavformat/raw.c
>> @@ -31,12 +31,11 @@
>>  
>>  /* simple formats */
>>  #if CONFIG_FLAC_MUXER
>> -static int flac_write_header(struct AVFormatContext *s)
>> +int ff_flac_write_header(ByteIOContext *pb, AVCodecContext *codec)
>>  {
> 
> i doubt a little that this belongs on raw.c/h

True, it would definitely make dependencies simpler if it were split it
into a separate file.  How about libavformat/flac.[ch]?

-Justin