[FFmpeg-devel] [PATCH 1/2] add support for ROI-based encoding

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Derek Buitenhuis
> Sent: Saturday, December 22, 2018 12:36 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] add support for ROI-based
> encoding
> 
> A few comments below.
> 
> On 12/12/2018 16:26, Guo, Yejun wrote:
> > +        if (frame->rois_buf != NULL) {
> > +            if (x4->params.rc.i_aq_mode == X264_AQ_NONE) {
> > +                av_log(ctx, AV_LOG_ERROR, "Adaptive quantization must
> > + be enabled to use ROI encoding, skipping ROI.\n");
> 
> This should, in my opinion, return an error and fail hard.
> 
> If people want it to continue anyway, it should be AV_LOG_WARNING.

thanks, WARNING is better, will change to it.

> 
> > +            } else {
> > +                if (frame->interlaced_frame == 0) {
> > +                    const static int MBSIZE = 16;
> 
> I think we generally use defines for this stuff.

will change to define.

> 
> > +                    size_t mbx = (frame->width + MBSIZE - 1) / MBSIZE;
> > +                    size_t mby = (frame->height + MBSIZE - 1) /
> > + MBSIZE;
> 
> 
> > +                    float* qoffsets = (float*)av_malloc(sizeof(float)
> > + * mbx * mby);
> 
> Convention in FFmpeg is to use sizeof(*var).

ok, I'll follow it.

> 
> > +                    memset(qoffsets, 0, sizeof(float) * mbx * mby);
> 
> Missing NULL check for alloc failure.

thanks, will add the check.

> 
> > +
> > +                    size_t nb_rois = frame->rois_buf->size /
> > + sizeof(AVFrameROI);
> 
> I think we have some macros that do this already.

I tried a code search and do not find one, there is a similar macro which does not work for this case: #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
I might miss it, I'll try again to see if any luck.

> 
> > +                    AVFrameROI* rois = (AVFrameROI*)frame->rois_buf->data;
> > +                    for (size_t roi = 0; roi < nb_rois; ++roi) {
> 
> Nit/convention: roi++

ok, will change.

> 
> > +                        int starty = FFMIN(mby, rois[roi].top / MBSIZE);
> > +                        int endy = FFMIN(mby, (rois[roi].bottom + MBSIZE - 1)/
> MBSIZE);
> > +                        int startx = FFMIN(mbx, rois[roi].left / MBSIZE);
> > +                        int endx = FFMIN(mbx, (rois[roi].right + MBSIZE - 1)/ MBSIZE);
> > +                        for (int y = starty; y < endy; ++y) {
> > +                            for (int x = startx; x < endx; ++x) {

and will also change to x++ and y ++

> > +                                qoffsets[x + y*mbx] = get_roi_qoffset(ctx,
> rois[roi].quality);
> > +                            }
> > +                        }
> > +                    }
> > +
> > +                    x4->pic.prop.quant_offsets = qoffsets;
> > +                    x4->pic.prop.quant_offsets_free = av_free;
> > +                } else {
> > +                    av_log(ctx, AV_LOG_ERROR, "interlaced_frame not
> > + supported for ROI encoding yet, skipping ROI.\n");
> 
> Same comment as befor: return error or change to warning.

will change to warning.

> 
> 
> 
> > +enum AVRoiQuality {
> 
> Probably should be AVROIQuality.
> 
> > +    AV_RQ_NONE = 0,
> > +    AV_RQ_BETTER = 1,
> > +    AV_RQ_BEST = 2,
> > +};
> > +
> > +typedef struct AVFrameROI {
> > +    /* coordinates at frame pixel level.
> > +     * it will be extended internally if the codec requirs an alignment
> > +     */
> > +    size_t top;
> > +    size_t bottom;
> > +    size_t left;
> > +    size_t right;
> > +    enum AVRoiQuality quality;
> > +} AVFrameROI;
> 
> Are we not going to allow the API to set an actual offset? This really limits
> what someone can do. (I say this as a user of x264's ROI API, in my own
> codebase, at least.)

thanks, the new idea is to use an integer value instead of the enum.

> 
> Cheers,
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel