[FFmpeg-devel] [PATCH] liavcodec: add bit-rate support to RoQ video encoder

[Tomas Härdin]

mån 2024-01-22 klockan 21:32 +0300 skrev Victor Luchitz:
> On Mon, Jan 22, 2024 at 4:57 PM Tomas Härdin <git at haerdin.se> wrote:
> 
> > > > 
> > > > >  -    if (frame->quality)
> > > > > -        enc->lambda = frame->quality - 1;
> > > > > -    else
> > > > > -        enc->lambda = 2*ROQ_LAMBDA_SCALE;
> > > > > +    if (avctx->bit_rate <= ROQ_DEFAULT_MIN_BIT_RATE) {
> > > > > +        /* no specific bit rate desired, use frame quality
> > > > > */
> > > > > +        if (frame->quality)
> > > > > +            enc->lambda = frame->quality - 1;
> > > > > +        else
> > > > > +            enc->lambda = 2*ROQ_LAMBDA_SCALE;
> > > > > +    }
> > > > 
> > > > This looks like a bit of a janky way to switch between qscale
> > > > and
> > > > bitrate. Isn't there a way to detect whether an option has been
> > > > set
> > > > explicitly? At the very least this behavior should be
> > > > documented in
> > > > doc/encoders.texi
> > > > 
> > > 
> > > Originally, the code just checked for bit_rate !=
> > > AV_CODEC_DEFAULT_BITRATE,
> > > which required including options_table.h, which in turn produced
> > > a
> > > bunch
> > > of compilation warnings about certain fields being deprecated.
> > > None
> > > of the
> > > other codecs include that file + many simply check the bit_rate
> > > field
> > > against
> > > magic constants.
> > 
> > grepping for 200000 didn't reveal anything like that. Do you have a
> > specific example of an encoder that does this?
> > 
> 
> ~/ffmpeg/libavcodec# grep -RI bit_rate . | grep 000 | grep -v 1000 |
> wc -l
> 38

Most of these seem to revolve around sane defaults, picking various
submodes or rejecting too high or too low bitrates


> Most of the cases here are comparisons against magic constants
> without
> any comments about how they were chosen or what they do, none
> whatsoever.

I would suggest adding a different default as Martin suggests. If users
really want shitty encodes I see no reason to deny them. I could see it
being useful for modders.

You might want an option for the tolerance stuff too

/Tomas