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

Michael Niedermayer michaelni
Sun Feb 22 02:18:26 CET 2009


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090222/62b1ed30/attachment.pgp>



More information about the ffmpeg-devel mailing list