[Ffmpeg-devel] [PATCH] Vorbis Encoder

[Michael Niedermayer]

Hi

On Sun, Oct 01, 2006 at 10:28:29AM +0200, Oded Shimon wrote:
> On Sun, Oct 01, 2006 at 10:04:43AM +0200, Michael Niedermayer wrote:
> > On Sat, Sep 30, 2006 at 11:33:49PM +0300, Oded Shimon wrote:
> > [..]
> > duplicate of nth_root() of vorbis.c
> [..]
> > this looks like a duplicate of vorbis_len2vlc() in vorbis.c
> [..]
> > that one looks like its part of vorbis_parse_setup_hdr_floors() from vorbis.c
> > please remove all code duplications between vorbis.c & vorbis_enc.c
> > ill review the rest after you removed all duplicate code
> 
> It's quite non trivial to remove all duplicate code between vorbis.c and 
> vorbis_enc.c, because even though the algos are the same, the structs are 

iam not saying its easy, but pretty much all other codecs in lavc also 
share common code


> completely different. The only way I can think of doing is by using common 
> struct names with similar members, and have these helper functions in 
> vorbis.h, and include vorbis.h AFTER the struct definitions in vorbis.c 
> and vorbis_enc.c (same code, but different compilation for different 
> files). This seems quite dirty, is this the way you intended?

no, the structures used by the common functions should be the same
not just have the same name and similarely named members

if this really isnt possible without having huge amounts of unused
fields in the structs (i doubt that) then you can do something similar 
to how AVFrame / Picture is implemenetd 


[...]
> > [...]
> > >     mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps);
> > >     mc->angle = av_malloc(sizeof(int) * mc->coupling_steps);
> > 
> > av_malloc(sizeof(*mc->angle) ...
> > 
> > would be safer in case the type changes
> 
> Should I change for all cases of av_malloc ?..

i would say yes, but its your decission, i wont reject the patch due
to that, but i will reject it if it duplicates half of vorbis.c


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is