[FFmpeg-devel] RoQ video encoder (take 3)
Sun Jun 10 22:23:19 CEST 2007
Michael Niedermayer wrote:
>> + 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.
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.
More information about the ffmpeg-devel