[FFmpeg-devel] [PATCH] flac demuxer

[Michael Niedermayer]

On Sat, May 03, 2008 at 09:26:29AM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Sat, May 03, 2008 at 07:24:51AM -0400, Justin Ruggles wrote:
> >> Michael Niedermayer wrote:
> >>> On Fri, May 02, 2008 at 06:21:35PM -0400, Justin Ruggles wrote:
> >>>> Michael Niedermayer wrote:
> >>>>> On Mon, Apr 28, 2008 at 12:51:10AM -0400, Justin Ruggles wrote:
> >>>>>> Justin Ruggles wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> This patch splits out the FLAC demuxer from raw.c.  It has added
> >>>>>>> functionality to read the raw FLAC header, including all metadata.  The
> >>>>>>> function to read the streaminfo header needs to be shared with the FLAC
> >>>>>>> decoder, so it has been put in lavc.
> >>>>>>>
> >>>>>>> * svn cp libavformat/raw.c libavformat/flacdec.c
> >>>> Here is a patch just to create a new flac demuxer file by copying the
> >>>> relevant functions from raw.c.  Each function will be modified later to
> >>>> be more specific to FLAC instead of the current generic raw demuxing.
> >>> copying (code duplication), and later changing part of it really sounds
> >>> ugly and wrong.
> >>>
> >>> If you want to split raw.c that surely can be done without any code
> >>> clone and mutate, also spliting raw.c has nothing to to with flac.
> >> I'm a bit confused. My goal is to put the flac demuxer in its own file
> >> so I can add flac-specific header parsing (eventually seeking) without
> >> cluttering up raw.c.  Would it be better to just write it from scratch
> >> all at once, along with removal from raw.c instead of copying?
> > 
> > The problem is the code duplication not how you end up with code duplication.
> > Explicitly copying code or implementing from scratch the same functions makes
> > no difference.
> > 
> > You can add whatever functions are needed to raw.c. If you dislike this you
> > have to split raw.c cleanly, copying whatever code you need into a new file
> > so it exists twice is absolutely not ok.
> 
> ok. I think I'm starting to see what you're getting at. A lot of the
> "skeleton" code of many of the demuxers is already the same. A proper
> raw flac demuxer which does full header parsing and correct seeking
> wouldn't really be much different except that there is already a
> sub-optimal demuxer existing in raw.c.  What it comes down to is whether
> the history should show an incremental rewrite (which is how I actually
> did it), a totally new demuxer, or something in-between.

If theres nothing in common between the final flac demuxer and raw.c then
IMHO it is cleaner to not svn cp from raw.c


> 
> Essentially, raw FLAC is a raw container, but it also has a specific
> header, including metadata and a seek table.  So the only function which
> would have substantial duplicated code is raw_read_partial_packet().  In
> fact the function could work just fine as-is, but it wouldn't be
> necessarily be ideal to always read 1024 bytes for each packet if you
> can determine the largest possible frame size from the header.  

> That
> said, would it be better to just share that one function and reuse it in
> a separate flac demuxer file?

yes i think so

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20080503/596354b7/attachment.pgp>