[FFmpeg-devel] [PATCH] fix raw FLAC muxer extradata handling
Justin Ruggles
justin.ruggles
Sat Feb 21 22:27:54 CET 2009
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-a-shared-function-to-validate-FLAC-extradata.patch
Type: text/x-diff
Size: 10439 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/01f9e11c/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Share-the-function-to-write-a-raw-FLAC-header-and-us.patch
Type: text/x-diff
Size: 5098 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/01f9e11c/attachment-0001.patch>
More information about the ffmpeg-devel
mailing list