diff mbox

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

Message ID 20190603231905.9536-2-sw@jkqxz.net
State Accepted
Commit d76e2aaf0896007771556e279ae85e825b81e7cc
Headers show

Commit Message

Mark Thompson June 3, 2019, 11:19 p.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 | 53 +++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

Comments

Guo, Yejun June 4, 2019, 6:33 a.m. UTC | #1 
> -----Original Message-----

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

> Mark Thompson

> Sent: Tuesday, June 04, 2019 7:19 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: [FFmpeg-devel] [PATCH v3 2/5] 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 | 53 +++++++++++++++++++++++++-------------------

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

> 

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

> index 45d07a92fc..dc4b4b100d 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,50 @@ 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;

> +                    uint32_t roi_size;

> +                    float *qoffsets;

> +

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

> +                    roi_size = roi->self_size;

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

> +                        av_log(ctx, AV_LOG_ERROR, "Invalid

> AVRegionOfInterest.self_size.\n");

> +                        return AVERROR(EINVAL);

> +                    }

> +                    nb_rois = sd->size / roi_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 = 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);

> +                    // 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_size * i);

> +

> +                        starty = FFMIN(mby, roi->top / MB_SIZE);

> +                        endy   = FFMIN(mby, (roi->bottom + MB_SIZE -

> 1)/ MB_SIZE);

> +                        startx = FFMIN(mbx, roi->left / MB_SIZE);

> +                        endx   = FFMIN(mbx, (roi->right + MB_SIZE -

> 1)/ MB_SIZE);

> +

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


LGTM, thanks.

> --

> 2.20.1

> 

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

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

> 

> To unsubscribe, visit link above, or email

> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 45d07a92fc..dc4b4b100d 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,50 @@  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;
+                    uint32_t roi_size;
+                    float *qoffsets;
+
+                    roi = (const AVRegionOfInterest*)sd->data;
+                    roi_size = roi->self_size;
+                    if (!roi_size || sd->size % roi_size != 0) {
+                        av_log(ctx, AV_LOG_ERROR, "Invalid AVRegionOfInterest.self_size.\n");
+                        return AVERROR(EINVAL);
+                    }
+                    nb_rois = sd->size / roi_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 = 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);
+                    // 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_size * i);
+
+                        starty = FFMIN(mby, roi->top / MB_SIZE);
+                        endy   = FFMIN(mby, (roi->bottom + MB_SIZE - 1)/ MB_SIZE);
+                        startx = FFMIN(mbx, roi->left / MB_SIZE);
+                        endx   = FFMIN(mbx, (roi->right + MB_SIZE - 1)/ MB_SIZE);
+
                         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;