[FFmpeg-devel] RoQ video encoder (take 3)

[Michael Niedermayer]

Hi

On Sun, Jun 10, 2007 at 04:23:19PM -0400, Eric Lasota wrote:
> Michael Niedermayer wrote:
[...]
> > why is this not a single normal fuction?
> >
> >   
> 
> For the same reason that the elbg.c patch was recently submitted, 
> because gcc 3.4.4 does not like multi-dimensional array parameters where 
> deeper dimensions are variable.  Doing the same workaround with an array 
> with two variable dimensions would be a major hack, less so than 
> templating the functions, and using a single-dimension array would be ugly.

IIRC these are just 1D arrays, the 2D structure is purely artifical and
does nothing but complicate the code

the sum of squared difference is the same no matter how you organize the
components be it 1D, 2D or 123D
so my question again why this macro and pages of code mess for a simple
block memcpy and a simple find the minimum SSD by brute force of a few
vectors?


> 
> Also, the encoder spends a lot of time in the squared_diff functions, 
> index_mb4, and index_mb8, so it may be worth it to micro-optimize them 
> for speed by using fixed sizes, assuming gcc doesn't aggressive-inline them.

1. write clean code
after that
2. high level optimizations
3. low level optimizations

i cant see any 2 and 3 in the code, just a macro with unoptimized code
it has a copy loop in c instead of memcpy(), theres no early termination
with decorrelation, no Kd trees, no asm
just a macro, which could as well have been done as (always_)inline function


> 
> > duplicated
> >   
> 
> add_to_size_calc is operate then increment, remove_from_size_calc is 
> decrement then operate.  I thought that was enough of a reason to leave 
> them separate instead of making a more-cryptic single generic function.

the macro makes it an order of magnitude more cryptic than replacing
x++;
by
x+= C;


[...]
> 
> > this seems redundant with gop_size and framesSinceKeyframe, also a design
> > based on the assumtion that the distance between keyframes is constant is
> > not ideal, some frame might very well be much cheaper to make keyframes
> > out of (first frame after a scene change for examplle) compared to the 
> > avergage frame ...
> > of course such scene change detection has nothing to do with the current
> > patch ...
> >
> >   
> 
> Keyframes in RoQ are so bad that I recommend encoding with -gop 99999.  
> They provide no benefit over P frames because they are P frames, they 
> just can't use motion compensation or skip blocks, and waste 3-11% of 
> their size on 2-bit typecodes that can only be 2 possible values.  I 
> included them in case under some scenario, users wanted RoQ videos 
> resistant to corruption, which is a dubious benefit given the 
> applications anyway, so improving keyframes is at the absolute bottom of 
> my priority list.  If anything, I'd rather see them removed completely

well keyframes are essential for random access / seeking

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070611/f9f4614f/attachment.pgp>