| Message ID | 1581651125-25232-1-git-send-email-yejun.guo@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/frame: add use_last_roi | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Fri, Feb 14, 2020 at 4:41 AM Guo, Yejun <yejun.guo@intel.com> wrote: > > For some cases, the regions of interest do not change, it is not > convenient to always prepare the roi data for every frame. So, add > use_last_roi to show it uses the same roi data as last frame. > > Since a new flag is added into AVFrame, the major version number of > lavu is changed. > Changing the major version for a field addition is not acceptable, it should be added to the end and not require that. Furthermore, I feel like the entire presence of that field in AVFrame is not a good idea. Its a very specific field, and we actively tried to remove such fields from the very generic structures like AVFrame, using side-data and other feature/codec-specific things instead. ROI itself is side-data, so this flag should be as well. - Hendrik
On 2/14/2020 5:33 AM, Hendrik Leppkes wrote: > On Fri, Feb 14, 2020 at 4:41 AM Guo, Yejun <yejun.guo@intel.com> wrote: >> >> For some cases, the regions of interest do not change, it is not >> convenient to always prepare the roi data for every frame. So, add >> use_last_roi to show it uses the same roi data as last frame. >> >> Since a new flag is added into AVFrame, the major version number of >> lavu is changed. >> > > Changing the major version for a field addition is not acceptable, it > should be added to the end and not require that. Furthermore, I feel > like the entire presence of that field in AVFrame is not a good idea. > Its a very specific field, and we actively tried to remove such fields > from the very generic structures like AVFrame, using side-data and > other feature/codec-specific things instead. ROI itself is side-data, > so this flag should be as well. This new field is also not acceptable because, same as the disposable flag i wanted to add some time ago, it is not really an attribute of a frame in itself, but of a frame in a frame chain. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Hi, On Thu, Feb 13, 2020 at 10:41 PM Guo, Yejun <yejun.guo@intel.com> wrote: > For some cases, the regions of interest do not change, it is not > convenient to always prepare the roi data for every frame. Since side-data is refcounted, can't you just keep the "last" one in memory and refcount it? That way, if you wanted to know if it's the same, you can compare pointers, or if you want to access the ROI data directly, you can do that, but it doesn't require a copy. Ronald
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of > James Almer > Sent: Friday, February 14, 2020 9:33 PM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutil/frame: add use_last_roi > > On 2/14/2020 5:33 AM, Hendrik Leppkes wrote: > > On Fri, Feb 14, 2020 at 4:41 AM Guo, Yejun <yejun.guo@intel.com> wrote: > >> > >> For some cases, the regions of interest do not change, it is not > >> convenient to always prepare the roi data for every frame. So, add > >> use_last_roi to show it uses the same roi data as last frame. > >> > >> Since a new flag is added into AVFrame, the major version number of > >> lavu is changed. > >> > > > > Changing the major version for a field addition is not acceptable, it > > should be added to the end and not require that. Furthermore, I feel > > like the entire presence of that field in AVFrame is not a good idea. > > Its a very specific field, and we actively tried to remove such fields > > from the very generic structures like AVFrame, using side-data and > > other feature/codec-specific things instead. ROI itself is side-data, > > so this flag should be as well. > > This new field is also not acceptable because, same as the disposable > flag i wanted to add some time ago, it is not really an attribute of a > frame in itself, but of a frame in a frame chain. thanks Hendrik and James. Understand the requirement for AVFrame now, will first try to resolve it with another method at application level. thanks.
> -----Original Message----- > From: Guo, Yejun > Sent: Tuesday, February 18, 2020 10:35 AM > To: Guo, Yejun <yejun.guo@intel.com> > Subject: RE: [FFmpeg-devel] [PATCH 1/3] avutil/frame: add use_last_roi > > > > From: Ronald S. Bultje [mailto:rsbultje@gmail.com] > Sent: Friday, February 14, 2020 11:02 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Cc: Guo, Yejun <yejun.guo@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutil/frame: add use_last_roi > > Hi, > >> On Thu, Feb 13, 2020 at 10:41 PM Guo, Yejun <yejun.guo@intel.com> wrote: >> For some cases, the regions of interest do not change, it is not >> convenient to always prepare the roi data for every frame. > > Since side-data is refcounted, can't you just keep the "last" one in memory and > refcount it? That way, if you wanted to know if it's the same, you can compare > pointers, or if you want to access the ROI data directly, you can do that, but it > doesn't require a copy. > > Ronald Thanks Ronald, yes, the application developers can 'cache' the last 'AVBufferRef *' of the roi side-data, so they don't need to prepare the same side-data for every frame, just pass 'AVBufferRef*' to side-data. So, we don't need to add use_last_roi in AVFrame. The only difference is that we need to re-calculate rois for encoders within ffmpeg, we can optimize it with your method if it is found to be a bottle neck.
On Tue, Feb 18, 2020 at 11:06 AM Guo, Yejun <yejun.guo@intel.com> wrote: > > > > > -----Original Message----- > > From: Guo, Yejun > > Sent: Tuesday, February 18, 2020 10:35 AM > > To: Guo, Yejun <yejun.guo@intel.com> > > Subject: RE: [FFmpeg-devel] [PATCH 1/3] avutil/frame: add use_last_roi > > > > > > > > From: Ronald S. Bultje [mailto:rsbultje@gmail.com] > > Sent: Friday, February 14, 2020 11:02 PM > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > Cc: Guo, Yejun <yejun.guo@intel.com> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutil/frame: add use_last_roi > > > > Hi, > > > >> On Thu, Feb 13, 2020 at 10:41 PM Guo, Yejun <yejun.guo@intel.com> wrote: > >> For some cases, the regions of interest do not change, it is not > >> convenient to always prepare the roi data for every frame. > > > > Since side-data is refcounted, can't you just keep the "last" one in memory and > > refcount it? That way, if you wanted to know if it's the same, you can compare > > pointers, or if you want to access the ROI data directly, you can do that, but it > > doesn't require a copy. > > > > Ronald > > Thanks Ronald, yes, the application developers can 'cache' the last 'AVBufferRef *' of > the roi side-data, so they don't need to prepare the same side-data for every frame, just > pass 'AVBufferRef*' to side-data. So, we don't need to add use_last_roi in AVFrame. > > The only difference is that we need to re-calculate rois for encoders within ffmpeg, we > can optimize it with your method if it is found to be a bottle neck. I don't think re-calculate rois per frame is a true bottle neck, but have a method to avoid unnecessary re-calculate is better.
Quoting Guo, Yejun (2020-02-14 04:32:05) > For some cases, the regions of interest do not change, it is not > convenient to always prepare the roi data for every frame. So, add > use_last_roi to show it uses the same roi data as last frame. > > Since a new flag is added into AVFrame, the major version number of > lavu is changed. Why is this even necessary at all? I believe the convention for the other side data types is that it applies from the frame it is attached to onwards.
diff --git a/doc/APIchanges b/doc/APIchanges index 761f37f..eec0f2f 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-02-14 - xxxxxxxxxx - lavu 57.0.100 - frame.h + Add use_last_roi in AVFrame. + 2020-02-13 - xxxxxxxxxx - lavu 56.41.100 - tx.h Add AV_TX_INT32_FFT and AV_TX_INT32_MDCT diff --git a/libavutil/frame.h b/libavutil/frame.h index b5afb58..2c786f4 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -634,6 +634,14 @@ typedef struct AVFrame { AVBufferRef *hw_frames_ctx; /** + * It is set if the RegionOfInterest info is the same as last frame, + * so, we don't need to prepare the same thing in side_data. + * If there is roi info in side_data and this variable is also set, + * use the info in side_data. + */ + int8_t use_last_roi; + + /** * AVBufferRef for free use by the API user. FFmpeg will never check the * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when * the frame is unreferenced. av_frame_copy_props() calls create a new diff --git a/libavutil/version.h b/libavutil/version.h index 90cc55b..aade19b 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -78,8 +78,8 @@ * @{ */ -#define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 41 +#define LIBAVUTIL_VERSION_MAJOR 57 +#define LIBAVUTIL_VERSION_MINOR 0 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ @@ -106,28 +106,28 @@ */ #ifndef FF_API_VAAPI -#define FF_API_VAAPI (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_VAAPI (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_FRAME_QP -#define FF_API_FRAME_QP (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_FRAME_QP (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_PLUS1_MINUS1 -#define FF_API_PLUS1_MINUS1 (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_PLUS1_MINUS1 (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_ERROR_FRAME -#define FF_API_ERROR_FRAME (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_ERROR_FRAME (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_PKT_PTS -#define FF_API_PKT_PTS (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_PKT_PTS (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_CRYPTO_SIZE_T -#define FF_API_CRYPTO_SIZE_T (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_CRYPTO_SIZE_T (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_FRAME_GET_SET -#define FF_API_FRAME_GET_SET (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_FRAME_GET_SET (LIBAVUTIL_VERSION_MAJOR < 58) #endif #ifndef FF_API_PSEUDOPAL -#define FF_API_PSEUDOPAL (LIBAVUTIL_VERSION_MAJOR < 57) +#define FF_API_PSEUDOPAL (LIBAVUTIL_VERSION_MAJOR < 58) #endif
For some cases, the regions of interest do not change, it is not convenient to always prepare the roi data for every frame. So, add use_last_roi to show it uses the same roi data as last frame. Since a new flag is added into AVFrame, the major version number of lavu is changed. Signed-off-by: Guo, Yejun <yejun.guo@intel.com> --- doc/APIchanges | 3 +++ libavutil/frame.h | 8 ++++++++ libavutil/version.h | 20 ++++++++++---------- 3 files changed, 21 insertions(+), 10 deletions(-)