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

[Eric Lasota]

Michael Niedermayer wrote:
> Hi
>   
>> +    uint32_t byteConsumption;    ///< Number of bytes used for data encodes consumed
>> +    uint32_t codeConsumption;    ///< Number of 2-bit codes consumed
>> +    uint32_t combinedBitConsumption;
>>     
>
> whats the sense of these 3?! a single variable will do just fine
> also they should be int or unsigned int
>
>   

Leftovers from when they were used in the size calc.

Also, one thing that may be worth revising (i.e. renaming) is that 
combinedBitConsumption isn't even a bit count, it's bits/2, because all 
possible values are 2-divisible, it's not used in the size calc, and it 
allows distortion variables to be higher-resolution without overflowing 
during the cross-multiplies.

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

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.

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

> this looks like a nasty missuse of the max/min rate variables
>
>   

It is, would you prefer just multiplying I frame rates by a constant 
(i.e. 2) instead until better rate control is added?

> also i dont think a constant number of bytes per frame is a good idea
> even less so considering the complexity of the current code (which sadly
> ive not completely reviewed yet, but i thought i better post some reply
> with a partial review instead of letting you wait for a week ...)
>   

You've said in earlier comments that it would be better to get a stable 
encoder in and work on linking it with ratecontrol.c later, which is 
something I'm working on.  I will say that most of the unnecessary 
complexity in terms of program flow has been eliminated, at least given 
the current way the reducer works.

Even if the fixed-size stuff is removed, I'd support leaving most of the 
existing code in because it could allow for more interesting rate 
control options, specifically that both target size and lambda can be 
used as controls, and either can be used to quickly evaluate the other.

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