[FFmpeg-devel] [PATCH 1/2] add support for ROI-based encoding

[Derek Buitenhuis]

A few comments below.

On 12/12/2018 16:26, Guo, Yejun wrote:
> +        if (frame->rois_buf != NULL) {
> +            if (x4->params.rc.i_aq_mode == X264_AQ_NONE) {
> +                av_log(ctx, AV_LOG_ERROR, "Adaptive quantization must be enabled to use ROI encoding, skipping ROI.\n");

This should, in my opinion, return an error and fail hard.

If people want it to continue anyway, it should be AV_LOG_WARNING.

> +            } else {
> +                if (frame->interlaced_frame == 0) {
> +                    const static int MBSIZE = 16;

I think we generally use defines for this stuff.

> +                    size_t mbx = (frame->width + MBSIZE - 1) / MBSIZE;
> +                    size_t mby = (frame->height + MBSIZE - 1) / MBSIZE;


> +                    float* qoffsets = (float*)av_malloc(sizeof(float) * mbx * mby);

Convention in FFmpeg is to use sizeof(*var).

> +                    memset(qoffsets, 0, sizeof(float) * mbx * mby);

Missing NULL check for alloc failure.

> +
> +                    size_t nb_rois = frame->rois_buf->size / sizeof(AVFrameROI);

I think we have some macros that do this already.

> +                    AVFrameROI* rois = (AVFrameROI*)frame->rois_buf->data;
> +                    for (size_t roi = 0; roi < nb_rois; ++roi) {

Nit/convention: roi++

> +                        int starty = FFMIN(mby, rois[roi].top / MBSIZE);
> +                        int endy = FFMIN(mby, (rois[roi].bottom + MBSIZE - 1)/ MBSIZE);
> +                        int startx = FFMIN(mbx, rois[roi].left / MBSIZE);
> +                        int endx = FFMIN(mbx, (rois[roi].right + MBSIZE - 1)/ MBSIZE);
> +                        for (int y = starty; y < endy; ++y) {
> +                            for (int x = startx; x < endx; ++x) {
> +                                qoffsets[x + y*mbx] = get_roi_qoffset(ctx, rois[roi].quality);
> +                            }
> +                        }
> +                    }
> +
> +                    x4->pic.prop.quant_offsets = qoffsets;
> +                    x4->pic.prop.quant_offsets_free = av_free;
> +                } else {
> +                    av_log(ctx, AV_LOG_ERROR, "interlaced_frame not supported for ROI encoding yet, skipping ROI.\n");

Same comment as befor: return error or change to warning.



> +enum AVRoiQuality {

Probably should be AVROIQuality.

> +    AV_RQ_NONE = 0,
> +    AV_RQ_BETTER = 1,
> +    AV_RQ_BEST = 2,
> +};
> +
> +typedef struct AVFrameROI {
> +    /* coordinates at frame pixel level.
> +     * it will be extended internally if the codec requirs an alignment
> +     */
> +    size_t top;
> +    size_t bottom;
> +    size_t left;
> +    size_t right;
> +    enum AVRoiQuality quality;
> +} AVFrameROI;

Are we not going to allow the API to set an actual offset? This really
limits what someone can do. (I say this as a user of x264's ROI API, in my
own codebase, at least.)

Cheers,
- Derek