Re: [FFmpeg-devel] [PATCH v3] lavc/libvpxenc: add screen-content-mode option

[Dariusz Marcinkiewicz via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

Hello.

On Fri, Feb 9, 2024 at 7:28 PM James Zern <jzern@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-request@ffmpeg.org with subject "unsubscribe".