Hello.
On Fri, Feb 9, 2024 at 7:28 PM James Zern <jz...@google.com> wrote:
>
Just sent v4, which addresses the below comments.
Thank you.
> On Thu, Feb 8, 2024 at 1:58 PM Dariusz Marcinkiewicz via ffmpeg-devel
> <ffmpeg-devel@ffmpeg.org> wrote:
...
> > --- a/libavcodec/libvpxenc.c
> > +++ b/libavcodec/libvpxenc.c
> > @@ -114,6 +114,7 @@ typedef struct VPxEncoderContext {
> > int crf;
> > int static_thresh;
> > int max_intra_rate;
> > + int screen_content_mode;
>
> Move this to a VP8-only section similar to VP9.
>
Done.
...
> > @@ -164,6 +165,7 @@ static const char *const ctlidstr[] = {
...
> > +#ifdef VPX_CTRL_VP8E_SET_SCREEN_CONTENT_MODE
>
> This control was available in libvpx 1.4.0, the minimum version
> supported. You can remove this check.
>
Done.
...
> > + if (avctx->codec_id == AV_CODEC_ID_VP8 && ctx->screen_content_mode >=
> > 0) {
> > + if (ctx->screen_content_mode == 2 && ctx->is_alpha) {
>
> Indent is 4 spaces here and throughout the patch.
>
Done.
> > [...]
> >
> > - coded_size = queue_frames(avctx, &ctx->encoder,
> > &ctx->coded_frame_list, pkt);
> > + coded_size = queue_frames(avctx, &ctx->encoder, &ctx->coded_frame_list,
> > + pkt, ctx->is_alpha, &frame_enc);
> > + if (avctx->codec_id == AV_CODEC_ID_VP8 && frame_enc == 0 &&
> > + ctx->screen_content_mode == 2 && frame) {
> > + // VP8 tuned for screen content with aggresive rate control -
> > returned
>
> aggressive.
>
> > + // OK status code but produced no output, this indicates frame was
> > + // rolled back due to bitrate overshoot - try to encode it again.
>
> This is a little weird given there's no adjustment to the encoder. I
> think this should be a separate patch at least. If the encoder decided
> to drop the frame in this mode it seems like the right decision given
> the setting description. If it works as part of the drop frames
> threshold then maybe the recode should be in the library based on some
> threshold.
>
Dropped this part.
...
> > [...]
> > @@ -1946,6 +1987,12 @@ static const AVOption vp8_options[] = {
> > { "auto-alt-ref", "Enable use of alternate reference "
> > "frames (2-pass only)",
> > OFFSET(auto_alt_ref), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, VE},
> > { "cpu-used", "Quality/Speed ratio modifier",
> > OFFSET(cpu_used), AV_OPT_TYPE_INT, {.i64 = 1}, -16, 16, VE},
> > +#ifdef VPX_CTRL_VP8E_SET_SCREEN_CONTENT_MODE
> > + { "screen-content-mode", "Encoder screen content mode",
> > OFFSET(screen_content_mode), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, VE,
> > "screen_content_mode"},
> > + { "off", "Screen content mode off", 0,
> > AV_OPT_TYPE_CONST, {.i64 = 0}, 0, 0, VE,
> > "screen_content_mode"},
> > + { "on", "Screen content mode on", 0,
> > AV_OPT_TYPE_CONST, {.i64 = 1}, 0, 0, VE,
> > "screen_content_mode"},
> > + { "on-agressive-rate-control", "Screen content mode on with aggressive
> > rate control", 0, AV_OPT_TYPE_CONST, {.i64 = 2}, 0, 0, VE,
> > "screen_content_mode"},
>
> aggressive.
> There's no string equivalent in vpxenc though, so this should probably
> just be ints.
>
Removed.
...
> > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > index 0fae3d06d3..4b618d740f 100644
> > --- a/libavcodec/version.h
> > +++ b/libavcodec/version.h
> > @@ -30,7 +30,7 @@
> > #include "version_major.h"
> >
> > #define LIBAVCODEC_VERSION_MINOR 38
> > -#define LIBAVCODEC_VERSION_MICRO 100
> > +#define LIBAVCODEC_VERSION_MICRO 101
> >
>
> This will need a rebase.
Done.
_______________________________________________
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".
