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

Aurelien Jacobs aurel
Sat Feb 21 15:52:09 CET 2009


On Fri, 20 Feb 2009 20:57:09 -0500
Justin Ruggles <justin.ruggles at gmail.com> 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
> 
> [...]
> 
> [0001-Use-a-shared-function-to-validate-FLAC-extradata.patch  text/x-diff (8.8KB)]
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index e5e87f9..25f20fc 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c

OK.

> [0004-Share-the-function-to-write-a-raw-FLAC-header-and-us.patch  text/x-diff (3.3KB)]
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 25f20fc..f0c8f57 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -24,6 +24,7 @@
>  #include "isom.h"
>  #include "matroska.h"
>  #include "avc.h"
> +#include "raw.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/md5.h"
>  #include "libavcodec/xiph.h"

I suspect you could remove the flac.h include along with this change.
Except that. This matroskaenc patch is OK too.

Aurel




More information about the ffmpeg-devel mailing list