[FFmpeg-devel] [PATCH] read header in FLAC demuxer
Michael Niedermayer
michaelni
Tue Feb 3 00:17:48 CET 2009
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
> > 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?
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
[...]
> @@ -304,6 +306,85 @@
> return 0;
> }
>
> +#ifdef CONFIG_FLAC_DEMUXER
my script says this is wrong and should be a #if
also static functions that are unused should not need #if unless they refer
to unavailable things and thus could fail compilation
> +static int flac_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> + int err, metadata_done=0, type, length, found_streaminfo=0, buffer_size=0;
> + uint32_t header;
> + uint8_t *buffer=NULL;
> + FLACStreaminfo si;
> + AVStream *st;
> +
> + err = audio_read_header(s, ap);
> + if (err)
> + return err;
> +
> + st = s->streams[0];
> +
> + /* if fLaC marker is not found, assume there is no header */
> + if (get_le32(s->pb) != MKTAG('f','L','a','C'))
> + return 0;
> +
> + /* process metadata blocks */
> + while (!url_feof(s->pb) && !metadata_done) {
> + header = get_be32(s->pb);
> + metadata_done = (header & 0x80000000);
> + type = (header & 0x7f000000) >> 24;
> + length = (header & 0x00ffffff);
> +
> + if (!buffer || buffer_size < length) {
> + buffer = av_realloc(buffer, length + FF_INPUT_BUFFER_PADDING_SIZE);
> + if (!buffer)
> + return AVERROR(ENOMEM);
memleak
> + buffer_size = length;
> + }
> + if (get_buffer(s->pb, buffer, length) != length) {
> + av_free(buffer);
> + return AVERROR(EIO);
> + }
> +
> + if (type == FLAC_METADATA_TYPE_STREAMINFO) {
> + if (length != FLAC_STREAMINFO_SIZE) {
> + av_free(buffer);
> + return AVERROR(EINVAL);
> + }
> + found_streaminfo = 1;
> + st->codec->extradata = buffer;
> + st->codec->extradata_size = length;
is there anything that prevents this from being executed several times and
thus leaking?
[...]
> + if (buffer)
> + av_free(buffer);
useless if()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/96900f6e/attachment.pgp>
More information about the ffmpeg-devel
mailing list