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

mypopy at gmail.com mypopy at gmail.com
Thu Feb 28 05:25:57 EET 2019


On Thu, Feb 28, 2019 at 10:53 AM Guo, Yejun <yejun.guo at intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> > Of Mark Thompson
> > Sent: Thursday, February 28, 2019 6:00 AM
> > To: ffmpeg-devel at ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH 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.
> >
> > 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);
>
> just found following from "$ ./x264 --fullhelp", not sure what 81 means
here. Shall we change our qoffset formula accordingly?
>       --qpmin <integer>       Set min QP [0]
>       --qpmax <integer>       Set max QP [81]
>
> >                      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.
>
> Nit:   the reason may be more straight forward.
> This list must be iterated in reverse because: when overlapping regions
are defined, the first region containing a given area of the frame applies.
>
> > +                    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);
>
> not quite understand why endx/endy is calculated so.
> For example, for a 1920x1080 frame, and roi->top is 0, and roi->bottom is
1080, then,
> starty is 0, endy is 0, which make the following loop does not work.

I think Mark use the (left/top) and (right/bottom) as the offset, like this:
in the fig, (left/top) == (s1x, s1y), (right/bottom) ==(s2x,s2y)


+-----------------------+------> w (x)
|          ^            |
|          | s1y        |
|          V            |
|      +***********+    |
| s1x  *           * s2x|
|<-->  *  ROI      *<-->|
|      *           *    |
|      +***********+    |
|          ^            |
|          | s2y        |
|          V            |
|-----------------------+
|
V

h (y)

>
>                           for (int y = starty; y < endy; y++) {
>                               for (int x = startx; x < endx; x++) {
>                                   qoffsets[x + y*mbx] = qoffset;
>                               }
>                           }
>
> > +
> >                          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
> >


More information about the ffmpeg-devel mailing list