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

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Tuesday, June 04, 2019 7:19 AM
> To: ffmpeg-devel at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".