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

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Derek Buitenhuis
> Sent: Thursday, February 28, 2019 1:11 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support
> for ROI-based encoding
> 
> On 27/02/2019 16:12, Guo, Yejun wrote:
> > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > ---
> >  libavcodec/libvpxenc.c | 132
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 132 insertions(+)
> 
> Do these APIs exist for all supported libvpx versions?

Yes, I checked release v1.0.0 at https://github.com/webmproject/libvpx/releases,
there is file examples/ vp8_set_maps.txt shows how to set ROI and active maps,
they use the same API as the current API.

> 
> > +    if (!sd) {
> > +        if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP,
> &active_map)) {
> > +            log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP
> codec control.\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> 
> Why do this stuff at all if no ROIs have been set?

For the case that some middle frames do not have ROI while other frames have ROIs.
Due to the fact of libvpx, we have to reset VP8E_SET_ACTIVEMAP, otherwise the previous ROIs continue working.
I'll add notes into the code, thanks.

> 
> > +    memset(active_map.active_map, 1, active_map.rows *
> active_map.cols);
> 
> Nit: Maybe a comment about what '1' is here.

will add, thanks.

> 
> > +
> > +    /* segment id 0 in roi_map is reserved for the areas not covered by
> AVRegionOfInterest.
> > +     * segment id 0 in roi_map is also for the areas with
> AVRegionOfInterest.qoffset near 0.
> > +     */
> > +    segment_id = 0;
> > +    segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
> > +    segment_id++;
> 
> This series of ops seems weirdly redundant separately?

yes, I want to show the mapping logic explicitly.  Let me make it simpler.

> 
> > +    memset(&roi_map, 0, sizeof(roi_map));
> 
> Can't zero-init?

I guess it not easy to zero-init with " vpx_roi_map_t roi_map = {...};", see the struct below.

typedef struct vpx_roi_map {
  /*! If ROI is enabled. */
  uint8_t enabled;
  /*! An id between 0-3 (0-7 for vp9) for each 16x16 (8x8 for VP9)
   * region within a frame. */
  unsigned char *roi_map;
  unsigned int rows; /**< Number of rows. */
  unsigned int cols; /**< Number of columns. */
  /*! VP8 only uses the first 4 segments. VP9 uses 8 segments. */
  int delta_q[8];  /**< Quantizer deltas. */
  int delta_lf[8]; /**< Loop filter deltas. */
  /*! skip and ref frame segment is only used in VP9. */
  int skip[8];      /**< Skip this block. */
  int ref_frame[8]; /**< Reference frame for this block. */
  /*! Static breakout threshold for each segment. Only used in VP8. */
  unsigned int static_threshold[4];
} vpx_roi_map_t;

> 
> > +        av_free(active_map.active_map);
> > +        av_log(avctx, AV_LOG_ERROR,
> > +               "roi_map alloc (%d bytes) failed.\n",
> > +               roi_map.rows * roi_map.cols);
> 
> Here, and elsewhere: We don't write how many bytes failed, generally.

I use this style because function queue_frames in the same file use it. 
anyway, I'll change it.

> 
> > +    if (vpx_codec_control(&ctx->encoder, VP8E_SET_ROI_MAP, &roi_map))
> {
> > +        av_free(active_map.active_map);
> > +        av_free(roi_map.roi_map);
> > +        log_encoder_error(avctx, "Failed to set VP8E_SET_ROI_MAP codec
> control.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP,
> &active_map)) {
> > +        av_free(active_map.active_map);
> > +        av_free(roi_map.roi_map);
> > +        log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec
> control.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    av_free(active_map.active_map);
> > +    av_free(roi_map.roi_map);
> 
> With the amount of failure areas in this function, is it reasonable to roll it
> into one goto fail or something?

good idea, I'll add it.

> 
> > +        if (avctx->codec_id == AV_CODEC_ID_VP8)
> > +            vp8_encode_set_roi(avctx, frame);
> 
> The API only exists for VP8, or is this due to different quant scales or
> something?

currently, VP9 ROI does not work, see my discussion at https://groups.google.com/a/webmproject.org/forum/#!topic/codec-devel/HVBRjoW0fTw,
the issue is confirmed by libvpx, and I'm helping to verify their new patch for the fix.

It is expected that VP9 ROI support will not be released in a short time, so I just add for vp8 roi. 
There is something common between VP8/VP9 ROI, and I plan to extract an common function when doing the work for VP9 roi.


btw, I'll also change the patch a little on how to iterate the AVRegionOfInterest array, because I think Mark Thompson's code is better than mine.

> 
> As an aside, I must say, libvpx's ROI API is real ugly.
> 
> Cheers,
> - Derek
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel