> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Derek Buitenhuis > Sent: Saturday, December 22, 2018 12:36 AM > To: ffmpeg-devel@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel