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

[Derek Buitenhuis]

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?

> +    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?

> +    memset(active_map.active_map, 1, active_map.rows * active_map.cols);

Nit: Maybe a comment about what '1' is here.

> +
> +    /* 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?

> +    memset(&roi_map, 0, sizeof(roi_map));

Can't zero-init?

> +        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.

> +    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?

> +        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?

As an aside, I must say, libvpx's ROI API is real ugly.

Cheers,
- Derek