[FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based encoding support for VP8/VP9 support

[Guo, Yejun]

> -----Original Message-----
> From: James Zern [mailto:jzern at google.com]
> Sent: Tuesday, August 27, 2019 12:03 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: Guo, Yejun <yejun.guo at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based
> encoding support for VP8/VP9 support
> 
> Hi,
> 
> On Thu, Aug 22, 2019 at 5:56 PM Guo, Yejun <yejun.guo at intel.com> wrote:
> >
> > example command line to verify it:
> > ./ffmpeg -i input.stream -vf addroi=0:0:iw/3:ih/3:-0.8 -c:v libvpx -b:v 2M
> tmp.webm
> >
> > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > ---
> >  libavcodec/libvpxenc.c | 194
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 194 insertions(+)
> >
> 
> Looks OK overall, just a few minor comments and you should bump the
> micro version number for this change.

thanks,
do you mean bump LIBAVCODEC_VERSION_MICRO in file libavcodec/version.h? thanks.

> 
> > [...]
> > +
> > +            segment_mapping[mapping_index] = segment_id + 1;
> > +            roi_map->delta_q[segment_id] = delta_q;
> > +            segment_id++;
> > +        }
> > +    }
> > +
> > +
> 
> This line can go.

do you mean the next line av_assert0 can be removed? thanks.

> 
> > [...]
> > +
> > +    av_assert0(!roi_supported);
> > +    if (!ctx->roi_warned) {
> > +        ctx->roi_warned = 1;
> > +        av_log(avctx, AV_LOG_WARNING, "ROI is not supported, please
> upgrade libvpx to version >= 1.8.1, "
> > +                                      "and you might need to build
> ffmpeg again.\n");
> 
> This could be "...to version >= 1.8.1. You may need to rebuild ffmpeg.\n"

thanks, will change.

> 
> > [...]
> > +
> > +        if (sd) {
> > +            if (avctx->codec_id == AV_CODEC_ID_VP8)
> > +                vp8_encode_set_roi(avctx, frame->width,
> frame->height, sd);
> > +            else if (avctx->codec_id == AV_CODEC_ID_VP9)
> 
> This is the only other option, it can just be 'else'. Other checks in
> the code assume this already.

ok, will only keep 'else'

> 
> > +                vp9_encode_set_roi(avctx, frame->width,
> frame->height, sd);
> > +        }
> >      }