[FFmpeg-devel] [PATCH] read header in FLAC demuxer

[Justin Ruggles]

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
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.

-Justin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac_read_header_8.diff
Type: text/x-diff
Size: 6912 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090202/3ddc1595/attachment.diff>