[FFmpeg-devel] [PATCH V6 2/2] avcodec/libx264: add support for ROI-based encoding

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Tuesday, January 08, 2019 3:20 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V6 2/2] avcodec/libx264: add support
> for ROI-based encoding
> 
> On Tue, Jan 08, 2019 at 03:54:38AM +0800, Guo, Yejun wrote:
> > This patch just enables the path from ffmpeg to libx264, the more
> > encoders can be added later.
> >
> > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > ---
> >  libavcodec/libx264.c | 67
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index
> > a68d0a7..9cfbaed 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -40,6 +40,10 @@
> >  #include <stdlib.h>
> >  #include <string.h>
> >
> > +// from x264.h, for quant_offsets, Macroblocks are 16x16 // blocks of
> > +pixels (with respect to the luma plane) #define MB_SIZE 16
> > +
> >  typedef struct X264Context {
> >      AVClass        *class;
> >      x264_param_t    params;
> > @@ -282,6 +286,7 @@ static int X264_frame(AVCodecContext *ctx,
> AVPacket *pkt, const AVFrame *frame,
> >      x264_picture_t pic_out = {0};
> >      int pict_type;
> >      int64_t *out_opaque;
> > +    AVFrameSideData *sd;
> >
> >      x264_picture_init( &x4->pic );
> >      x4->pic.img.i_csp   = x4->params.i_csp;
> > @@ -345,6 +350,68 @@ static int X264_frame(AVCodecContext *ctx,
> AVPacket *pkt, const AVFrame *frame,
> >                  }
> >              }
> >          }
> > +
> > +        sd = av_frame_get_side_data(frame,
> AV_FRAME_DATA_REGIONS_OF_INTEREST);
> > +        if (sd) {
> 
> > +            static int show_aq_warning = 1;
> > +            static int show_interlaced_frame_warning = 1;
> 
> non constant statics are a bad idea.
> there is not neccesarily just a single x264 encoder

ok, I'll revert it.

> 
> 
> > +            if (x4->params.rc.i_aq_mode == X264_AQ_NONE &&
> show_aq_warning) {
> > +                show_aq_warning = 0;
> > +                av_log(ctx, AV_LOG_WARNING, "Adaptive quantization must be
> enabled to use ROI encoding, skipping ROI.\n");
> > +            } else {
> > +                if (frame->interlaced_frame == 0) {
> > +                    int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
> > +                    int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
> > +                    int nb_rois;
> > +                    AVRegionOfInterest* roi;
> > +                    float* qoffsets;
> 
> > +                    qoffsets = (float*)av_mallocz_array(mbx * mby,
> > + sizeof(*qoffsets));
> 
> the cast is unneeded

thanks, will remove it.

> 
> 
> 
> > +                    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);
> 
> vertical alignment makes this more readable

yes, will refine it.

> 
> 
> > +                        float qoffset;
> > +
> > +                        if (roi->qoffset.den == 0) {
> > +                            av_free(qoffsets);
> > +                            av_log(ctx, AV_LOG_ERROR,
> "AVRegionOfInterest.qoffset.den should not be zero.\n");
> > +                            return AVERROR(EINVAL);
> > +                        }
> > +                        qoffset = roi->qoffset.num * 1.0f /
> > + roi->qoffset.den;
> 
> > +                        qoffset = FFMIN(qoffset, 1.0f);
> > +                        qoffset = FFMAX(qoffset, -1.0f);
> 
> av_clipf()

thanks, will change to it.

> 
> > +
> > +                        // 25 is a number that I think it is a possible proper scale value.
> > +                        qoffset = qoffset * 25;
> > +
> > +                        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;
> > +                    x4->pic.prop.quant_offsets_free = av_free;
> > +                } else if (show_interlaced_frame_warning) {
> > +                    show_interlaced_frame_warning = 0;
> > +                    av_log(ctx, AV_LOG_WARNING, "interlaced_frame not
> supported for ROI encoding yet, skipping ROI.\n");
> > +                }
> > +            }
> > +        }
> >      }
> >
> >      do {
> > --
> > 2.7.4
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> --
> Michael     GnuPG fingerprint:
> 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein