diff mbox series

[FFmpeg-devel,V5,3/5] libavutil: add side data AV_FRAME_DATA_BOUNDING_BOXES

Message ID 20210308050522.9396-3-yejun.guo@intel.com
State New
Headers show
Series [FFmpeg-devel,V5,1/5] libavfilter/dnn: add ff_dnn_set_proc to set pre/post proc
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Guo, Yejun March 8, 2021, 5:05 a.m. UTC
Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
 doc/APIchanges    | 2 ++
 libavutil/frame.c | 1 +
 libavutil/frame.h | 5 +++++
 3 files changed, 8 insertions(+)

Comments

Anton Khirnov March 8, 2021, 8:33 a.m. UTC | #1
Quoting Guo, Yejun (2021-03-08 06:05:20)
> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> ---
>  doc/APIchanges    | 2 ++
>  libavutil/frame.c | 1 +
>  libavutil/frame.h | 5 +++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 4027d599e7..b83409a412 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,8 @@ libavutil:     2017-10-21
>  
>  
>  API changes, most recent first:
> +2021-03-xx - xxxxxxxxxx - lavu 56.xx.100 - frame.h
> +  Add AV_FRAME_DATA_BOUNDING_BOXES
>  
>  2021-03-04 - xxxxxxxxxx - lavc 58.128.101 - avcodec.h
>    Enable err_recognition to be set for encoders.
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index eab51b6a32..53868a2410 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -852,6 +852,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>      case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
>      case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
>      case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
> +    case AV_FRAME_DATA_BOUNDING_BOXES:              return "Bounding boxes";
>      }
>      return NULL;
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 1aeafef6de..3fbe15c47e 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -198,6 +198,11 @@ enum AVFrameSideDataType {
>       * Must be present for every frame which should have film grain applied.
>       */
>      AV_FRAME_DATA_FILM_GRAIN_PARAMS,
> +
> +    /**
> +     * The data is for internal use by libavfilter.

I do not like this. We should not have internal side data unless there
is a strong reason for it. There is nothing about this side data that
makes it inherently lavfi-specific.
Lynne March 8, 2021, 1:18 p.m. UTC | #2
Mar 8, 2021, 09:33 by anton@khirnov.net:

> Quoting Guo, Yejun (2021-03-08 06:05:20)
>
>> Signed-off-by: Guo, Yejun <>> yejun.guo@intel.com>> >
>> ---
>>  doc/APIchanges    | 2 ++
>>  libavutil/frame.c | 1 +
>>  libavutil/frame.h | 5 +++++
>>  3 files changed, 8 insertions(+)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 4027d599e7..b83409a412 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,8 @@ libavutil:     2017-10-21
>>  
>>  
>>  API changes, most recent first:
>> +2021-03-xx - xxxxxxxxxx - lavu 56.xx.100 - frame.h
>> +  Add AV_FRAME_DATA_BOUNDING_BOXES
>>  
>>  2021-03-04 - xxxxxxxxxx - lavc 58.128.101 - avcodec.h
>>  Enable err_recognition to be set for encoders.
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index eab51b6a32..53868a2410 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -852,6 +852,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>>  case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
>>  case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
>>  case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
>> +    case AV_FRAME_DATA_BOUNDING_BOXES:              return "Bounding boxes";
>>  }
>>  return NULL;
>>  }
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 1aeafef6de..3fbe15c47e 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -198,6 +198,11 @@ enum AVFrameSideDataType {
>>  * Must be present for every frame which should have film grain applied.
>>  */
>>  AV_FRAME_DATA_FILM_GRAIN_PARAMS,
>> +
>> +    /**
>> +     * The data is for internal use by libavfilter.
>>
>
> I do not like this. We should not have internal side data unless there
> is a strong reason for it. There is nothing about this side data that
> makes it inherently lavfi-specific.
>

I object to making this public. This is not something we want to be
stuck with. That's why I suggested internal_ref was used instead,
but unfortunately someone just noticed it exists, of course though
"but what if we need it for something else later, even though we've
done just fine without it until now!?!".
I still want to have internal_ref be used for this until the format
is stable enough and/or codecs which make use of this appear.
Or at least keep this patch as-is.
Anton Khirnov March 8, 2021, 1:31 p.m. UTC | #3
Quoting Lynne (2021-03-08 14:18:15)
> Mar 8, 2021, 09:33 by anton@khirnov.net:
> 
> > Quoting Guo, Yejun (2021-03-08 06:05:20)
> >
> >> Signed-off-by: Guo, Yejun <>> yejun.guo@intel.com>> >
> >> ---
> >>  doc/APIchanges    | 2 ++
> >>  libavutil/frame.c | 1 +
> >>  libavutil/frame.h | 5 +++++
> >>  3 files changed, 8 insertions(+)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index 4027d599e7..b83409a412 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -14,6 +14,8 @@ libavutil:     2017-10-21
> >>  
> >>  
> >>  API changes, most recent first:
> >> +2021-03-xx - xxxxxxxxxx - lavu 56.xx.100 - frame.h
> >> +  Add AV_FRAME_DATA_BOUNDING_BOXES
> >>  
> >>  2021-03-04 - xxxxxxxxxx - lavc 58.128.101 - avcodec.h
> >>  Enable err_recognition to be set for encoders.
> >> diff --git a/libavutil/frame.c b/libavutil/frame.c
> >> index eab51b6a32..53868a2410 100644
> >> --- a/libavutil/frame.c
> >> +++ b/libavutil/frame.c
> >> @@ -852,6 +852,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
> >>  case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
> >>  case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
> >>  case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
> >> +    case AV_FRAME_DATA_BOUNDING_BOXES:              return "Bounding boxes";
> >>  }
> >>  return NULL;
> >>  }
> >> diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> index 1aeafef6de..3fbe15c47e 100644
> >> --- a/libavutil/frame.h
> >> +++ b/libavutil/frame.h
> >> @@ -198,6 +198,11 @@ enum AVFrameSideDataType {
> >>  * Must be present for every frame which should have film grain applied.
> >>  */
> >>  AV_FRAME_DATA_FILM_GRAIN_PARAMS,
> >> +
> >> +    /**
> >> +     * The data is for internal use by libavfilter.
> >>
> >
> > I do not like this. We should not have internal side data unless there
> > is a strong reason for it. There is nothing about this side data that
> > makes it inherently lavfi-specific.
> >
> 
> I object to making this public. This is not something we want to be
> stuck with.

Why not? What about this API should be changed and why not make this
change now?

> That's why I suggested internal_ref was used instead,
> but unfortunately someone just noticed it exists, of course though
> "but what if we need it for something else later, even though we've
> done just fine without it until now!?!".
> I still want to have internal_ref be used for this until the format
> is stable enough and/or codecs which make use of this appear.
> Or at least keep this patch as-is.

I see no reason to believe the format will become more public-worthy
with time.
Nicolas George March 8, 2021, 3:32 p.m. UTC | #4
Anton Khirnov (12021-03-08):
> I see no reason to believe the format will become more public-worthy
> with time.

Just the fact that as months pass, the likelihood we notice something
that requires a deep change decreases.

Do you have any actual argument against internal side data, beyond "I do
not like it"?

Regards,
Anton Khirnov March 8, 2021, 7:44 p.m. UTC | #5
Quoting Nicolas George (2021-03-08 16:32:08)
> Anton Khirnov (12021-03-08):
> > I see no reason to believe the format will become more public-worthy
> > with time.
> 
> Just the fact that as months pass, the likelihood we notice something
> that requires a deep change decreases.
> 

As does the likelihood that anybody will actually do any work to polish
this.

> Do you have any actual argument against internal side data, beyond "I do
> not like it"?

The argument for internal side data is literally that it's not good
enough to be made public. So the option of making it private is used as
an excuse to avoid designing the format properly.
Lavfi is enough of a black box as it is, no need to make it even more so
with internal side data formats.
Paul B Mahol March 9, 2021, 8:45 a.m. UTC | #6
Unacceptable. Filter should use frame metadata instead.

Stop avoiding this crucial subject.

On Mon, Mar 8, 2021 at 8:44 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Nicolas George (2021-03-08 16:32:08)
> > Anton Khirnov (12021-03-08):
> > > I see no reason to believe the format will become more public-worthy
> > > with time.
> >
> > Just the fact that as months pass, the likelihood we notice something
> > that requires a deep change decreases.
> >
>
> As does the likelihood that anybody will actually do any work to polish
> this.
>
> > Do you have any actual argument against internal side data, beyond "I do
> > not like it"?
>
> The argument for internal side data is literally that it's not good
> enough to be made public. So the option of making it private is used as
> an excuse to avoid designing the format properly.
> Lavfi is enough of a black box as it is, no need to make it even more so
> with internal side data formats.
>
> --
> Anton Khirnov
> _______________________________________________
> 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".
Nicolas George March 9, 2021, 9:48 a.m. UTC | #7
Anton Khirnov (12021-03-08):
> As does the likelihood that anybody will actually do any work to polish
> this.

Fair enough.

> The argument for internal side data is literally that it's not good
> enough to be made public. So the option of making it private is used as
> an excuse to avoid designing the format properly.

This reasoning would apply to using private_ref as well. So I understand
your objection is against any solution that avoids properly designing
the structure. I can easily agree with that.

> Lavfi is enough of a black box as it is, no need to make it even more so
> with internal side data formats.

Please, there nothing of a black box about libavfilter.

Regards,
Anton Khirnov March 9, 2021, 6:08 p.m. UTC | #8
Quoting Nicolas George (2021-03-09 10:48:50)
> Anton Khirnov (12021-03-08):
> > The argument for internal side data is literally that it's not good
> > enough to be made public. So the option of making it private is used as
> > an excuse to avoid designing the format properly.
> 
> This reasoning would apply to using private_ref as well. So I understand
> your objection is against any solution that avoids properly designing
> the structure. I can easily agree with that.

Correct, I object to using private_ref even more strongly than to
private side data.

I would prefer private_ref to be removed entirely.

All this said, it would be acceptable to make this new side data private
(or declared unstable) for a clearly limited period of time, e.g. 3
months or until the next major bump or so. As long as it's not internal
indefinitely.

> 
> > Lavfi is enough of a black box as it is, no need to make it even more so
> > with internal side data formats.
> 
> Please, there nothing of a black box about libavfilter.

This is not even my claim, just something I've seen other people state
repeatedly. People find the tight coupling between filters and graphs to
be limiting.
Nicolas George March 9, 2021, 6:13 p.m. UTC | #9
Anton Khirnov (12021-03-09):
> Correct, I object to using private_ref even more strongly than to
> private side data.
> 
> I would prefer private_ref to be removed entirely.
> 
> All this said, it would be acceptable to make this new side data private
> (or declared unstable) for a clearly limited period of time, e.g. 3
> months or until the next major bump or so. As long as it's not internal
> indefinitely.

Then we are on the same page.

> This is not even my claim, just something I've seen other people state
> repeatedly. People find the tight coupling between filters and graphs to
> be limiting.

I would like to hear their arguments. Please let me know if you find
more accurate instances of these complaints.

Regards,
Guo, Yejun March 10, 2021, 2:56 a.m. UTC | #10
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nicolas
> George
> Sent: 2021年3月10日 2:13
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V5 3/5] libavutil: add side data
> AV_FRAME_DATA_BOUNDING_BOXES
> 
> Anton Khirnov (12021-03-09):
> > Correct, I object to using private_ref even more strongly than to
> > private side data.
> >
> > I would prefer private_ref to be removed entirely.
> >
> > All this said, it would be acceptable to make this new side data
> > private (or declared unstable) for a clearly limited period of time,
> > e.g. 3 months or until the next major bump or so. As long as it's not
> > internal indefinitely.
> 
> Then we are on the same page.
> 

Thanks all for your valuable inputs. I'll pursue the final solution for a public new
side data. I'll just make this patch set pending, and continue the development for
tensorflow object detection support and other filters such as classify, and also ask
for more feedbacks from potential customers. The new patch set will be sent out
once it is mature enough, at least in my opinion.
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 4027d599e7..b83409a412 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,8 @@  libavutil:     2017-10-21
 
 
 API changes, most recent first:
+2021-03-xx - xxxxxxxxxx - lavu 56.xx.100 - frame.h
+  Add AV_FRAME_DATA_BOUNDING_BOXES
 
 2021-03-04 - xxxxxxxxxx - lavc 58.128.101 - avcodec.h
   Enable err_recognition to be set for encoders.
diff --git a/libavutil/frame.c b/libavutil/frame.c
index eab51b6a32..53868a2410 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -852,6 +852,7 @@  const char *av_frame_side_data_name(enum AVFrameSideDataType type)
     case AV_FRAME_DATA_VIDEO_ENC_PARAMS:            return "Video encoding parameters";
     case AV_FRAME_DATA_SEI_UNREGISTERED:            return "H.26[45] User Data Unregistered SEI message";
     case AV_FRAME_DATA_FILM_GRAIN_PARAMS:           return "Film grain parameters";
+    case AV_FRAME_DATA_BOUNDING_BOXES:              return "Bounding boxes";
     }
     return NULL;
 }
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 1aeafef6de..3fbe15c47e 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -198,6 +198,11 @@  enum AVFrameSideDataType {
      * Must be present for every frame which should have film grain applied.
      */
     AV_FRAME_DATA_FILM_GRAIN_PARAMS,
+
+    /**
+     * The data is for internal use by libavfilter.
+     */
+    AV_FRAME_DATA_BOUNDING_BOXES,
 };
 
 enum AVActiveFormatDescription {