[FFmpeg-devel] [PATCH] read header in FLAC demuxer
Michael Niedermayer
michaelni
Tue Feb 3 03:12:18 CET 2009
On Mon, Feb 02, 2009 at 09:00:05PM -0500, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Mon, Feb 02, 2009 at 05:21:45PM -0500, Justin Ruggles wrote:
> >> Justin Ruggles wrote:
> >>> Justin Ruggles wrote:
> >>>> Michael Niedermayer wrote:
> >>>>> On Sat, Jan 31, 2009 at 10:47:53AM -0500, Justin Ruggles wrote:
> >>>>>> M?ns Rullg?rd wrote:
> >>>>>>> Justin Ruggles <justin.ruggles at gmail.com> writes:
> >>>>>>>
> >>>>>>>> M?ns Rullg?rd wrote:
> >>>>>>>>> Michael Niedermayer <michaelni at gmx.at> writes:
> >>>>>>>>>
> >>>>>>>>>> On Fri, Jan 30, 2009 at 10:36:10PM -0500, Justin Ruggles wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> Here is a patch to read the header in the raw FLAC demuxer. Currently
> >>>>>>>>>>> the header is passed through in the output AVPackets, which is not good
> >>>>>>>>>>> and requires some ugliness on the decoder side. After this patch is
> >>>>>>>>>>> applied, the decoder can be changed so it doesn't have to handle the
> >>>>>>>>>>> header except in the extradata. The extradata handling in the decoder
> >>>>>>>>>>> will also be simplified.
> >>>>>>>>>> is it allowed to place the header in the stream instead of extradata in
> >>>>>>>>>> flac in avi?
> >>>>>>>>> If it is physically possible, someone will do it.
> >>>>>>>> Should we really support everything that is physically possible?
> >>>>>>> Of course not.
> >>>>>>>
> >>>>>>>> FLAC in avi and wav should not have the header in the stream.
> >>>>>>> Flac in avi and wav simply shouldn't IMO. In fact, avi shouldn't at
> >>>>>>> all, and wav is only useful for PCM.
> >>>>>>>
> >>>>>>>>>> flac in mkv?
> >>>>>>>>> No.
> >>>>>>>>>
> >>>>>>>>>> flac in * ?
> >>>>>>>>> There's always Ogg...
> >>>>>>>> Ogg doesn't use the raw FLAC header. It splits it up in its own funny
> >>>>>>>> way, which we already support.
> >>>>>>>>
> >>>>>>>> Do we have any other format that is header+frames that we allow the
> >>>>>>>> header to be in the stream?
> >>>>>>> It's possible to do all sorts of funny things with MPEG-PS/TS. Some
> >>>>>>> variants of AAC encapsulation put (parts of) the header outside the
> >>>>>>> main stream, some put it all inline.
> >>>>>> But raw FLAC doesn't do any funny things... There is a header, which
> >>>>>> contains metadata, information about the stream, seek table, etc... It
> >>>>>> is always completely separate from the audio frames. If we only handle
> >>>>>> it in the parser or decoder, we lose the ability to use that information
> >>>>>> in the demuxer, right?
> >>>>> i dont see why handling the seektable or others in the raw demuxer would
> >>>>> affect if its passed on or not to the parser, of course theres no point
> >>>>> on passing the seektable to the parser but still one could if one wanted
> >>>> That is true. The issue is code duplication. It's either in lavf where
> >>>> we can better utilize it, lavc where we can't, or both. It would require
> >>>> messy workarounds to get it to work in one place with both and maybe
> >>>> even unavoidable code duplication. Passing the header would also mean
> >>>> that a FLAC parser would have to handle the header as well.
> >>>>
> >>>>> About the rest of the header i dont know how or where to handle it best
> >>>>> but what you apparently propose is not adding any features its not making
> >>>>> code simpler it just removes support for having the headers inline
> >>>> This does add features. It reads the metadata and sets the stream
> >>>> timebase and duration.
> >>>>
> >>>>> If you want to add support for something iam very much in favor for that
> >>>>> and if you explain how that might conflict with inline headers i also
> >>>>> dont mind loosing support for them assuming such files are practically
> >>>>> non existent but
> >>>>> Just removing it (or even worse in this patch duplicating it first)
> >>>>> with no real explanation why is something that feels odd ...
> >>>>> also moving code should never be split in duplication and removial
> >>>> Ok, I can update the patch to remove support in the decoder at the same
> >>>> time.
> >>> Here is an updated patch which also removes reading of the header in the
> >>> decoder. The whole header can still be handled as extradata, but that is
> >>> much simpler because it just extracts the STREAMINFO, which has a fixed
> >>> size and position.
> >>>
> >
> >>> I tried to implement a solution which allowed for reading the header
> >>> within the demuxer and inline in the decoder. I got it working, but it
> >>> was very messy (mainly due to ByteIOContext vs. GetBitContext) and I
> >
> > for()
> > get_buffer()
> > readfrombuffer()
> >
> > vs.
> > for()
> > readfrombuffer()
> >
> > isnt messy
>
> The issue is that in the demuxer, I have to read each metadata block
> header to know how much data to get. So I'm essentially already
> processing the header at that point. The demuxer and decoder do
> different things with the metadata, so the only function that can really
> be shared between them is already shared, ff_flac_parse_streaminfo(). I
> tried to share more code and just ended up with more code without really
> getting rid of much duplication.
>
> >>> could not avoid some code duplication. I don't see an advantage to
> >>> handling the header inline within the decoder, as the necessary
> >>> information should be passed in the extradata. It is, however, useful
> >>> in the demuxer for reading the metadata and stream parameters. If I
> >>> decide to attempt seeking support, reading the seek table in the demuxer
> >>> will be simple this way as well.
> >> oops... old patch. here is a new one against current SVN.
> >>
> >> -Justin
> >>
> >
> >> Index: libavcodec/flacdec.c
> >> ===================================================================
> >> --- libavcodec/flacdec.c (revision 16957)
> >> +++ libavcodec/flacdec.c (working copy)
> >> @@ -96,7 +96,6 @@
> >> }
> >>
> >> static void allocate_buffers(FLACContext *s);
> >> -static int metadata_parse(FLACContext *s);
> >>
> >> static av_cold int flac_decode_init(AVCodecContext *avctx)
> >> {
> >> @@ -105,16 +104,22 @@
> >>
> >> avctx->sample_fmt = SAMPLE_FMT_S16;
> >>
> >> - if (avctx->extradata_size > 4) {
> >> + if (avctx->extradata_size >= FLAC_STREAMINFO_SIZE) {
> >> /* initialize based on the demuxer-supplied streamdata header */
> >> - if (avctx->extradata_size == FLAC_STREAMINFO_SIZE) {
> >> + int streaminfo_start = 0;
> >> + if (avctx->extradata_size >= FLAC_STREAMINFO_SIZE + 8 &&
> >> + AV_RB32(avctx->extradata) == MKBETAG('f','L','a','C')) {
> >> + int type = avctx->extradata[4] & 0x7F;
> >> + int size = AV_RB24(&avctx->extradata[5]);
> >> + if (type != FLAC_METADATA_TYPE_STREAMINFO || size != FLAC_STREAMINFO_SIZE) {
> >> + av_log(avctx, AV_LOG_ERROR, "invalid FLAC extradata\n");
> >> + return -1;
> >> + }
> >> + streaminfo_start = 8;
> >> + }
> >> ff_flac_parse_streaminfo(avctx, (FLACStreaminfo *)s,
> >> - avctx->extradata);
> >> + &avctx->extradata[streaminfo_start]);
> >> allocate_buffers(s);
> >> - } else {
> >> - init_get_bits(&s->gb, avctx->extradata, avctx->extradata_size*8);
> >> - metadata_parse(s);
> >> - }
> >> }
> >>
> >> return 0;
> >
> > why are these changes needed?
>
> Currently, the decoder has a function to read through all the metadata
> blocks, but only really uses the STREAMINFO. It has to read through
> them all for when the header is inline and the GetBitContext needs to be
> moved past the header to get to the first frame. The same function is
> currently used for extradata, but it's not really necessary to read the
> whole header in that case. So the change just extracts the STREAMINFO
> for the case when the whole header is passed in the extradata.
more specific question
> >> + streaminfo_start = 8;
> >> + }
> >> ff_flac_parse_streaminfo(avctx, (FLACStreaminfo *)s,
> >> - avctx->extradata);
> >> + &avctx->extradata[streaminfo_start]);
why this change?
is the format of extradata changed? what effect has this on muxers and
demuxers?
>
> > also this patch must be tested against all odd flac in whatever files we
> > have to make sure there are no regressions, this change has a high potential
> > to break things
>
> We don't really have much odd FLAC that I'm aware of, but I have tried
> the files I could find in our samples collection and have tried creating
> some odd files of my own to test with. I haven't had any issues so far
> that don't also occur with the current decoder...for different reasons.
> I've tested with large metadata, small metadata, ogg, mkv... I don't
> have any samples for caf/qt but I found how it's supposed to be handled
> in the XiphQT code, and the headers are not sent inline. The
> extradata/"cookie" can contain either just the STREAMINFO or all metadata.
>
> I haven't tried many broken/fuzzed files. The two I did try exited
> cleanly with "I/O error occurred. Usually that means that input file is
> truncated and/or corrupted."
>
> I'm open to suggestions or other samples to test.
what you tested sounds good
in addition you could test if -acodec copy to containers supporting flac
result in identical files
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20090203/0678346d/attachment.pgp>
More information about the ffmpeg-devel
mailing list