diff mbox

[FFmpeg-devel,v2,3/6] libx264: Update ROI behaviour to match documentation

Message ID 20190313001748.11781-3-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson March 13, 2019, 12:17 a.m. UTC 
Fix the quantisation offset - use the whole range, and don't change the
offset size based on bit depth.

Iterate the list in reverse order.  The first region in the list is the one
that applies in the case of overlapping regions.
---
 libavcodec/libx264.c | 51 ++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

Comments

Guo, Yejun March 13, 2019, 1:16 a.m. UTC | #1 
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Wednesday, March 13, 2019 8:18 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: [FFmpeg-devel] [PATCH v2 3/6] libx264: Update ROI behaviour to

> match documentation

> 

> Fix the quantisation offset - use the whole range, and don't change the

> offset size based on bit depth.

> 

> Iterate the list in reverse order.  The first region in the list is the one

> that applies in the case of overlapping regions.

> ---

>  libavcodec/libx264.c | 51 ++++++++++++++++++++++++--------------------

>  1 file changed, 28 insertions(+), 23 deletions(-)

> 

> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c

> index 87e6fe7c94..f5b9b8b821 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,48 @@ 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;


need a variable to record self_size of the first roi, see reason below.
uint32_t self_size;

> +

> +                    roi = (const AVRegionOfInterest*)sd->data;


self_size = roi->self_size;

> +                    if (!roi->self_size || sd->size % roi->self_size != 0) {


if (!self_size || sd->size % self_size != 0) {

and other cases for self_size

> +                        av_log(ctx, AV_LOG_ERROR, "Invalid

> AVRegionOfInterest.self_size.\n");

> +                        return AVERROR(EINVAL);

> +                    }

> +                    nb_rois = sd->size / roi->self_size;

> +

>                      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 = av_clip(roi->y / MB_SIZE, 0, mby);

> -                        int endy   = av_clip((roi->y + roi->height + MB_SIZE - 1) /

> MB_SIZE, 0, mby);

> -                        int startx = av_clip(roi->x / MB_SIZE, 0, mbx);

> -                        int endx   = av_clip((roi->x + roi->width  + MB_SIZE - 1) /

> MB_SIZE, 0, mbx);

> +                    // This list must be iterated in reverse because the first

> +                    // region in the list applies when regions overlap.

> +                    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);


here is the reason, roi->self_size is not verified when roi changes with below code.

> +

> +                        starty = av_clip(roi->y / MB_SIZE, 0, mby);

> +                        endy   = av_clip((roi->y + roi->height + MB_SIZE - 1) / MB_SIZE,

> 0, mby);

> +                        startx = av_clip(roi->x / MB_SIZE, 0, mbx);

> +                        endx   = av_clip((roi->x + roi->width  + 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);

> -                        }


the check is removed, so we just need to verify and use the first ROI self_size.

> -                        roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);

>                      }

> 

>                      x4->pic.prop.quant_offsets = qoffsets;

> --

> 2.19.2

> 

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 87e6fe7c94..f5b9b8b821 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,48 @@  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;
+
+                    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;
+
                     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 = av_clip(roi->y / MB_SIZE, 0, mby);
-                        int endy   = av_clip((roi->y + roi->height + MB_SIZE - 1) / MB_SIZE, 0, mby);
-                        int startx = av_clip(roi->x / MB_SIZE, 0, mbx);
-                        int endx   = av_clip((roi->x + roi->width  + MB_SIZE - 1) / MB_SIZE, 0, mbx);
+                    // This list must be iterated in reverse because the first
+                    // region in the list applies when regions overlap.
+                    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->y / MB_SIZE, 0, mby);
+                        endy   = av_clip((roi->y + roi->height + MB_SIZE - 1) / MB_SIZE, 0, mby);
+                        startx = av_clip(roi->x / MB_SIZE, 0, mbx);
+                        endx   = av_clip((roi->x + roi->width  + 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;