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

[Victor Luchitz <vluchits@gmail.com>]

On Mon, Jan 22, 2024 at 4:06 PM Tomas Härdin <git@haerdin.se> wrote:

> mån 2024-01-22 klockan 00:08 +0300 skrev Victor Luchits:
> > One can now use the bitrate option (-b) to specify bit rate of the
> > video
> > stream in the RoQ encoder. The option only becomes effective for
> > values
> > above 800kbit/s, which is roughly equivalent to bandwidth of a 1x-
> > speed
> > CD-ROM drive, minus the bandwidth taken up by stereo DPCM stream.
> > Values
> > below this threshold produce visually inadequate results.
> >
> > Original patch by Joseph Fenton aka Chilly Willy
> >
> > Signed-off-by: Victor Luchits <vluchits@gmail.com>
> > ---
> >   Changelog                |   1 +
> >   libavcodec/roqvideo.h    |   1 +
> >   libavcodec/roqvideodec.c |  15 +++++
> >   libavcodec/roqvideoenc.c | 118 ++++++++++++++++++++++++++++++++++--
> > ---
> >   libavcodec/version.h     |   2 +-
> >   5 files changed, 123 insertions(+), 14 deletions(-)
>
> Fails to apply with git am on current master
>
> > warning: Patch sent with format=flowed; space at the end of lines
> > might be lost.
> > Applying: liavcodec: add bit-rate support to RoQ video encoder
> > error: corrupt patch at line 20
> > Patch failed at 0001 liavcodec: add bit-rate support to RoQ video
> > encoder
>
> Typo: liavcodec
>
> > +    /* Keyframe when no MOT or FCC codes in frame */
> > +    if (s->key_frame) {
> > +        av_log(avctx, AV_LOG_VERBOSE, "\nFound keyframe!\n");
> > +        rframe->pict_type = AV_PICTURE_TYPE_I;
> > +        avpkt->flags |= AV_PKT_FLAG_KEY;
>
> Consider resetting framesSinceKeyframe here
>

Thanks, good catch, gonna look into that.


>
> >  -    if (avctx->width > 65535 || avctx->height > 65535) {
>
> The leading space is probably what makes the patch not apply
>

Found it, thanks.


>
> > -        av_log(avctx, AV_LOG_ERROR, "Dimensions are max %d\n", enc-
> > >quake3_compat ? 32768 : 65535);
> > +    if (enc->quake3_compat && ((avctx->width > 32767 || avctx-
> > >height > 32767))) {
> > +        av_log(avctx, AV_LOG_ERROR, "Dimensions are max %d\n",
> > 32767);
> > +        return AVERROR(EINVAL);
> > +    }
> > +    else if (avctx->width > 65535 || avctx->height > 65535) {
> > +        av_log(avctx, AV_LOG_ERROR, "Dimensions are max %d\n",
> > 65535);
> >          return AVERROR(EINVAL);
> >      }
> >  -    if (((avctx->width)&(avctx->width-1))||((avctx->height)&(avctx-
> > >height-1)))
> > +    if (enc->quake3_compat && ((avctx->width)&(avctx->width-
> > 1))||((avctx->height)&(avctx->height-1)))
> >          av_log(avctx, AV_LOG_ERROR, "Warning: dimensions not power
> > of two, this is not supported by quake\n");
>
> These changes appear to be unrelated to bitrate. Consider separating
> them into a separate patch.
>

Noted.


>
> >  -    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. That made me assume that including options_table.h
goes against the common practice.


>
> /Tomas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>


-- 
Best regards,
 Victor Luchitz
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".