[FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to match documentation

[Mark Thompson]

Fix the quantisation offset - use the whole range, and don't change the
offset size based on bit depth.

Use correct bottom/right edge locations (they are offsets from bottom/right,
not from top/left).

Iterate the list in reverse order.  The regions are in order of decreasing
importance, so the most important must be applied last.
---
 libavcodec/libx264.c | 50 ++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index a3493f393d..475719021e 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -285,16 +285,18 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
     int nnal, i, ret;
     x264_picture_t pic_out = {0};
     int pict_type;
+    int bit_depth;
     int64_t *out_opaque;
     AVFrameSideData *sd;
 
     x264_picture_init( &x4->pic );
     x4->pic.img.i_csp   = x4->params.i_csp;
 #if X264_BUILD >= 153
-    if (x4->params.i_bitdepth > 8)
+    bit_depth = x4->params.i_bitdepth;
 #else
-    if (x264_bit_depth > 8)
+    bit_depth = x264_bit_depth;
 #endif
+    if (bit_depth > 8)
         x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;
     x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
 
@@ -359,45 +361,47 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
                 if (frame->interlaced_frame == 0) {
                     int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
                     int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
+                    int qp_range = 51 + 6 * (bit_depth - 8);
                     int nb_rois;
-                    AVRegionOfInterest* roi;
-                    float* qoffsets;
+                    const AVRegionOfInterest *roi;
+                    float *qoffsets;
                     qoffsets = av_mallocz_array(mbx * mby, sizeof(*qoffsets));
                     if (!qoffsets)
                         return AVERROR(ENOMEM);
 
-                    nb_rois = sd->size / sizeof(AVRegionOfInterest);
-                    roi = (AVRegionOfInterest*)sd->data;
-                    for (int count = 0; count < nb_rois; count++) {
-                        int starty = FFMIN(mby, roi->top / MB_SIZE);
-                        int endy   = FFMIN(mby, (roi->bottom + MB_SIZE - 1)/ MB_SIZE);
-                        int startx = FFMIN(mbx, roi->left / MB_SIZE);
-                        int endx   = FFMIN(mbx, (roi->right + MB_SIZE - 1)/ MB_SIZE);
+                    roi = (const AVRegionOfInterest*)sd->data;
+                    if (!roi->self_size || sd->size % roi->self_size != 0) {
+                        av_log(ctx, AV_LOG_ERROR, "Invalid AVRegionOfInterest.self_size.\n");
+                        return AVERROR(EINVAL);
+                    }
+                    nb_rois = sd->size / roi->self_size;
+
+                    // This list must be iterated in reverse because regions are
+                    // defined in order of decreasing importance.
+                    for (int i = nb_rois - 1; i >= 0; i--) {
+                        int startx, endx, starty, endy;
                         float qoffset;
 
+                        roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
+
+                        starty = av_clip(roi->top / MB_SIZE, 0, mby);
+                        endy   = av_clip((frame->height - roi->bottom + MB_SIZE - 1) / MB_SIZE, 0, mby);
+                        startx = av_clip(roi->left / MB_SIZE, 0, mbx);
+                        endx   = av_clip((frame->width - roi->right + MB_SIZE - 1) / MB_SIZE, 0, mbx);
+
                         if (roi->qoffset.den == 0) {
                             av_free(qoffsets);
-                            av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den should not be zero.\n");
+                            av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den must not be zero.\n");
                             return AVERROR(EINVAL);
                         }
                         qoffset = roi->qoffset.num * 1.0f / roi->qoffset.den;
-                        qoffset = av_clipf(qoffset, -1.0f, 1.0f);
-
-                        // 25 is a number that I think it is a possible proper scale value.
-                        qoffset = qoffset * 25;
+                        qoffset = av_clipf(qoffset * qp_range, -qp_range, +qp_range);
 
                         for (int y = starty; y < endy; y++) {
                             for (int x = startx; x < endx; x++) {
                                 qoffsets[x + y*mbx] = qoffset;
                             }
                         }
-
-                        if (roi->self_size == 0) {
-                            av_free(qoffsets);
-                            av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size should be set to sizeof(AVRegionOfInterest).\n");
-                            return AVERROR(EINVAL);
-                        }
-                        roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
                     }
 
                     x4->pic.prop.quant_offsets = qoffsets;
-- 
2.19.2