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

["Tomas Härdin" <git@haerdin.se>]

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

>  -    if (avctx->width > 65535 || avctx->height > 65535) {

The leading space is probably what makes the patch not apply

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

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

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