On Mon, Jan 22, 2024 at 4:06 PM Tomas Härdin <g...@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 <vluch...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
