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

[Justin Ruggles]

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

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