On 12/12/2018 16:26, Guo, Yejun wrote: > This patchset contains two patches. > - the first patch (this patch) finished the code and ask for upstream. > - the second patch is just a quick example on how to generate ROI info. > > The encoders such as libx264 support different QPs offset for different MBs, > it makes possible for ROI-based encoding. It makes sense to add support > within ffmpeg to generate/accept ROI infos and pass into encoders. > > Typical usage: After AVFrame is decoded, a ffmpeg filter or user's code > generates ROI info for that frame, and the encoder finally does the > ROI-based encoding. And so I choose to maintain the ROI info (AVFrameROI) > within AVFrame struct. > > Since the ROI info generator might more focus on the domain knowledge of > the interest regions, instead of the encoding detail, the AVFrameROI is > designed to be more friend for ffmpeg users. > > This patch just enabled the path from ffmpeg to libx264, the more encoders > can be added later. > > Signed-off-by: Guo, Yejun <yejun....@intel.com> > --- > libavcodec/libx264.c | 56 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > libavutil/frame.c | 8 ++++++++ > libavutil/frame.h | 24 ++++++++++++++++++++++ > 3 files changed, 88 insertions(+) > > ... > diff --git a/libavutil/frame.c b/libavutil/frame.c > index 9b3fb13..dbc4b0a 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -425,6 +425,13 @@ FF_DISABLE_DEPRECATION_WARNINGS > FF_ENABLE_DEPRECATION_WARNINGS > #endif > > + av_buffer_unref(&dst->rois_buf); > + if (src->rois_buf) { > + dst->rois_buf = av_buffer_ref(src->rois_buf); > + if (!dst->rois_buf) > + return AVERROR(ENOMEM); > + } > + > av_buffer_unref(&dst->opaque_ref); > av_buffer_unref(&dst->private_ref); > if (src->opaque_ref) { > @@ -571,6 +578,7 @@ FF_DISABLE_DEPRECATION_WARNINGS > FF_ENABLE_DEPRECATION_WARNINGS > #endif > > + av_buffer_unref(&frame->rois_buf); > av_buffer_unref(&frame->hw_frames_ctx); > > av_buffer_unref(&frame->opaque_ref); > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 66f27f4..00d509d 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -193,6 +193,23 @@ typedef struct AVFrameSideData { > AVBufferRef *buf; > } AVFrameSideData; > > +enum AVRoiQuality { > + AV_RQ_NONE = 0, > + AV_RQ_BETTER = 1, > + AV_RQ_BEST = 2, > +};
I don't think I like this enum - an integer value without directly specifying this meaning would seem best for future flexibility? Negative values might be meaningful. A greater range would also allow ranking if there are many regions, which may be needed if some are discarded (because of hardware constraints, for example - VAAPI makes this visible). > + > +typedef struct AVFrameROI { > + /* coordinates at frame pixel level. > + * it will be extended internally if the codec requirs an alignment > + */ What is intended to happen if regions overlap? (In the above you have it using the last value in the list.) > + size_t top; > + size_t bottom; > + size_t left; > + size_t right; > + enum AVRoiQuality quality; > +} AVFrameROI; > + > /** > * This structure describes decoded (raw) audio or video data. > * > @@ -556,6 +573,13 @@ typedef struct AVFrame { > attribute_deprecated > AVBufferRef *qp_table_buf; > #endif > + > + /** > + * For ROI-based encoding, the number of ROI area is implied > + * in the size of buf. > + */ > + AVBufferRef *rois_buf; This should be a new type of AVFrameSideData, not a new field in AVFrame. > + > /** > * For hwaccel-format frames, this should be a reference to the > * AVHWFramesContext describing the frame. > - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel