| 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 | expand |
| 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 |
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.
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.
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.
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,
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.
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".
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,
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.
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,
> -----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 --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 {
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(+)