> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
