diff mbox series

[FFmpeg-devel,1/3] avutil/frame: add use_last_roi

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Guo, Yejun Feb. 14, 2020, 3:32 a.m. UTC
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(-)

Comments

Hendrik Leppkes Feb. 14, 2020, 8:33 a.m. UTC | #1
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
James Almer Feb. 14, 2020, 1:33 p.m. UTC | #2
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".
>
Ronald S. Bultje Feb. 14, 2020, 3:01 p.m. UTC | #3
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
Guo, Yejun Feb. 18, 2020, 2:33 a.m. UTC | #4
> -----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.
Guo, Yejun Feb. 18, 2020, 3:06 a.m. UTC | #5
> -----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.
mypopy@gmail.com Feb. 18, 2020, 5:37 a.m. UTC | #6
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.
Anton Khirnov Feb. 18, 2020, 1:14 p.m. UTC | #7
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 mbox series

Patch

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