| Message ID | 20210301132053.30264-2-yejun.guo@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,V4,1/4] libavfilter/bbox.h: add BoundingBoxHeader and BoundingBox | 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 |
Guo, Yejun (12021-03-01): > private_ref is for internal use by a single libav* library. > It has to be NULL when ownership of the frame leaves the respective library, > > buffersink is the last step when the frame leaves libavfilter, so add unref here. > > Signed-off-by: Guo, Yejun <yejun.guo@intel.com> > --- > libavfilter/buffersink.c | 1 + > 1 file changed, 1 insertion(+) NAK. buffersink did not take ownership of that reference, therefore it does not own it and cannot unref it. If this change actually fixes something, i.e. if there is a ref at this point, then we need to find who put it there, because they are responsible for freeing it. Regards,
> -----Original Message----- > From: Nicolas George <george@nsup.org> > Sent: 2021年3月1日 21:36 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Cc: Guo, Yejun <yejun.guo@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > Guo, Yejun (12021-03-01): > > private_ref is for internal use by a single libav* library. > > It has to be NULL when ownership of the frame leaves the respective > > library, > > > > buffersink is the last step when the frame leaves libavfilter, so add unref here. > > > > Signed-off-by: Guo, Yejun <yejun.guo@intel.com> > > --- > > libavfilter/buffersink.c | 1 + > > 1 file changed, 1 insertion(+) > > NAK. buffersink did not take ownership of that reference, therefore it does not > own it and cannot unref it. > > If this change actually fixes something, i.e. if there is a ref at this point, then we > need to find who put it there, because they are responsible for freeing it. > Thanks for the review, as we talked at http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276728.html, private_ref is allocated in filter vf_dnn_detect.c for detected results, and other filters such as vf_drawbox/text (in plan) will read it. Since private_ref is for a single libav* library, it has to be NULL when it leaves libavfilter, so I have to unref it at the last step of libavfilter, that's in buffersink. Any other suggestion? thanks.
Guo, Yejun (12021-03-01): > Thanks for the review, as we talked at http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276728.html, > private_ref is allocated in filter vf_dnn_detect.c for detected results, and other > filters such as vf_drawbox/text (in plan) will read it. > > Since private_ref is for a single libav* library, it has to be NULL when it leaves libavfilter, > so I have to unref it at the last step of libavfilter, that's in buffersink. Any other suggestion? thanks. Sorry, I did not have this reply in my archives, I somehow missed it. What this is basically saying is that private_ref in libavfilter will be appropriated for the only use of bounding boxes. I think it is a significantly worse solution than using side data as you originally considered. Regards,
Just use frame metadata. On Mon, Mar 1, 2021 at 3:24 PM Nicolas George <george@nsup.org> wrote: > Guo, Yejun (12021-03-01): > > Thanks for the review, as we talked at > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276728.html, > > private_ref is allocated in filter vf_dnn_detect.c for detected results, > and other > > filters such as vf_drawbox/text (in plan) will read it. > > > > Since private_ref is for a single libav* library, it has to be NULL when > it leaves libavfilter, > > so I have to unref it at the last step of libavfilter, that's in > buffersink. Any other suggestion? thanks. > > Sorry, I did not have this reply in my archives, I somehow missed it. > > What this is basically saying is that private_ref in libavfilter will be > appropriated for the only use of bounding boxes. I think it is a > significantly worse solution than using side data as you originally > considered. > > Regards, > > -- > Nicolas George > _______________________________________________ > 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".
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nicolas > George > Sent: 2021年3月1日 22:24 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > Guo, Yejun (12021-03-01): > > Thanks for the review, as we talked at > > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276728.html, > > private_ref is allocated in filter vf_dnn_detect.c for detected > > results, and other filters such as vf_drawbox/text (in plan) will read it. > > > > Since private_ref is for a single libav* library, it has to be NULL > > when it leaves libavfilter, so I have to unref it at the last step of libavfilter, > that's in buffersink. Any other suggestion? thanks. > > Sorry, I did not have this reply in my archives, I somehow missed it. > > What this is basically saying is that private_ref in libavfilter will be > appropriated for the only use of bounding boxes. I think it is a significantly > worse solution than using side data as you originally considered. yes, this is what we did earlier. Actually, I think private_ref in libavfilter can only be used for an exclusive usage at a time. As Paul mentioned, I think AVFrame.metadata is a better choice.
Guo, Yejun (12021-03-01): > Actually, I think private_ref in libavfilter can only > be used for an exclusive usage at a time. Exactly. If we use it for this, then we cannot use for anything else in libavfilter. This use seems too specific to warrant dedicating such an unique field to it, even though we do not have a better use in sight. > As Paul mentioned, I think AVFrame.metadata is a better choice. If you can express it as a string or set of strings with a clear syntax that can easily be parsed, then possibly, yes. Regards,
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nicolas > George > Sent: 2021年3月1日 23:07 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > Guo, Yejun (12021-03-01): > > Actually, I think private_ref in libavfilter can only be used for an > > exclusive usage at a time. > > Exactly. If we use it for this, then we cannot use for anything else in libavfilter. > This use seems too specific to warrant dedicating such an unique field to it, > even though we do not have a better use in sight. > > > As Paul mentioned, I think AVFrame.metadata is a better choice. > > If you can express it as a string or set of strings with a clear syntax that can > easily be parsed, then possibly, yes. ooo, it is not easy to express the bounding boxes as strings in AVDictionaryEntry.value, the bounding box has several data members, and they are data and have high possibility to contain '\0' in the middle of the data. So, we might not use AVFrame.metadata. So, where to put the bounding boxes (object detection result generated by vf_dnn_detect), I now see several possible methods which all have positive/negative comments: 1. Add into side data The final result is to be in side data since it might be used by new encoders in the future, but this method changes the API. 1.1 We just add a new enum for side data, and keep the .h file (for structs) internal at first. There's comment that this is not allowed. (I personally prefer this one.) 1.2 We add enum for side data, and also export the .h file as part of FFmpeg API. The risk is that we might change the structs in .h file later, it breaks API. We need a versioning management for the struct, just like film grain as explained at http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276586.html . 2. Use private_ref Use private_ref for bounding boxes at first, and then change to side data when it is required. The disadvantage is that during the period, we cannot use it for anything else in libavfilter. any comment or any other suggestion? thanks.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Guo, > Yejun > Sent: 2021年3月1日 23:47 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Nicolas George > > Sent: 2021年3月1日 23:07 > > To: FFmpeg development discussions and patches > > <ffmpeg-devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: > > unref private_ref when frame leaves libavfilter > > > > Guo, Yejun (12021-03-01): > > > Actually, I think private_ref in libavfilter can only be used for an > > > exclusive usage at a time. > > > > Exactly. If we use it for this, then we cannot use for anything else in > libavfilter. > > This use seems too specific to warrant dedicating such an unique field > > to it, even though we do not have a better use in sight. > > > > > As Paul mentioned, I think AVFrame.metadata is a better choice. > > > > If you can express it as a string or set of strings with a clear > > syntax that can easily be parsed, then possibly, yes. > > ooo, it is not easy to express the bounding boxes as strings in > AVDictionaryEntry.value, the bounding box has several data members, and > they are data and have high possibility to contain '\0' in the middle of the data. > So, we might not use AVFrame.metadata. > > So, where to put the bounding boxes (object detection result generated by > vf_dnn_detect), I now see several possible methods which all have > positive/negative comments: > > 1. Add into side data > The final result is to be in side data since it might be used by new encoders in > the future, but this method changes the API. > > 1.1 We just add a new enum for side data, and keep the .h file (for structs) > internal at first. > There's comment that this is not allowed. (I personally prefer this one.) > > 1.2 We add enum for side data, and also export the .h file as part of FFmpeg > API. > The risk is that we might change the structs in .h file later, it breaks API. > We need a versioning management for the struct, just like film grain as > explained at > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276586.html . > > 2. Use private_ref > Use private_ref for bounding boxes at first, and then change to side data when > it is required. > The disadvantage is that during the period, we cannot use it for anything else > in libavfilter. > > any comment or any other suggestion? thanks. > Hi, I think option 2 might be the good choice for now. The background is that: to support video analytics in libavfilter, we need a place to put the analytics result (it is bounding box in this example for object detection) in AVFrame, and so the data can be transferred between filters, (it is possible that the data need to be transferred between filters and codecs in the future, since new codecs are going to support AI labels). The options describe the possible places for the data. For option 1.1, it is not allowed because .h file (structs for bounding box) for side data is not exported. For option 1.2, to avoid the possible risk of API breaking, we need a versioning management, and the code such as BoundingBoxV1 and BoundingBoxV2 etc. is not elegant. For option 2, private_ref in libavfilter can be used to transfer data between filters. And we don't see other usages now, except for the bounding box. So, imho, we can use private_ref for now, and move to side data once the structs are mature enough, and so no versioning management is required. And the private_ref can be used for other usage at that time.
Guo, Yejun (12021-03-04): > For option 1.1, it is not allowed because .h file (structs for bounding box) for side > data is not exported. > > For option 1.2, to avoid the possible risk of API breaking, we need a versioning > management, and the code such as BoundingBoxV1 and BoundingBoxV2 etc. is > not elegant. Have you considered option 1.3: 1.3. Add the enum for a new side data, but document: /** * The data is for internal use by libavfilter. */ For the users, it is equivalent to storing it in private_ref, since it is private, but it is closer to the good long-term API where the information is eventually available outside. Regards,
On Mon, Mar 1, 2021 at 4:46 PM Guo, Yejun <yejun.guo@intel.com> wrote: > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Nicolas > > George > > Sent: 2021年3月1日 23:07 > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: > unref > > private_ref when frame leaves libavfilter > > > > Guo, Yejun (12021-03-01): > > > Actually, I think private_ref in libavfilter can only be used for an > > > exclusive usage at a time. > > > > Exactly. If we use it for this, then we cannot use for anything else in > libavfilter. > > This use seems too specific to warrant dedicating such an unique field > to it, > > even though we do not have a better use in sight. > > > > > As Paul mentioned, I think AVFrame.metadata is a better choice. > > > > If you can express it as a string or set of strings with a clear syntax > that can > > easily be parsed, then possibly, yes. > > ooo, it is not easy to express the bounding boxes as strings in > AVDictionaryEntry.value, > the bounding box has several data members, and they are data and have high > possibility > to contain '\0' in the middle of the data. So, we might not use > AVFrame.metadata. > What your bounding box actually have? I thought it is just few numbers and string describing box, no? > > So, where to put the bounding boxes (object detection result generated by > vf_dnn_detect), > I now see several possible methods which all have positive/negative > comments: > > 1. Add into side data > The final result is to be in side data since it might be used by new > encoders in the future, > but this method changes the API. > > 1.1 We just add a new enum for side data, and keep the .h file (for > structs) internal at first. > There's comment that this is not allowed. (I personally prefer this one.) > > 1.2 We add enum for side data, and also export the .h file as part of > FFmpeg API. > The risk is that we might change the structs in .h file later, it breaks > API. > We need a versioning management for the struct, just like film grain as > explained > at http://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276586.html . > > 2. Use private_ref > Use private_ref for bounding boxes at first, and then change to side data > when it is required. > The disadvantage is that during the period, we cannot use it for anything > else in libavfilter. > > any comment or any other suggestion? thanks. > > _______________________________________________ > 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".
Quoting Guo, Yejun (2021-03-04 09:48:14)
> Hi, I think option 2 might be the good choice for now.
Given past experience, I am very much not a fan of "for now" solutions.
They tend to ossify and remain in place for years (I have seen at least
one "//temporary hack" remain unchanged for over 10 years). Then further
hacks are grown on these "for now" solutions and they become very hard
to change or remove.
So I would strongly prefer that you spend the effort now to design a
proper future-proof solution (most likely side data based). If it turns
out to need some change later, then there's plenty of precedent for
changing API.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Paul B > Mahol > Sent: 2021年3月4日 17:26 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > On Mon, Mar 1, 2021 at 4:46 PM Guo, Yejun <yejun.guo@intel.com> wrote: > > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Nicolas > > > George > > > Sent: 2021年3月1日 23:07 > > > To: FFmpeg development discussions and patches > > > <ffmpeg-devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: > > unref > > > private_ref when frame leaves libavfilter > > > > > > Guo, Yejun (12021-03-01): > > > > Actually, I think private_ref in libavfilter can only be used for > > > > an exclusive usage at a time. > > > > > > Exactly. If we use it for this, then we cannot use for anything else > > > in > > libavfilter. > > > This use seems too specific to warrant dedicating such an unique > > > field > > to it, > > > even though we do not have a better use in sight. > > > > > > > As Paul mentioned, I think AVFrame.metadata is a better choice. > > > > > > If you can express it as a string or set of strings with a clear > > > syntax > > that can > > > easily be parsed, then possibly, yes. > > > > ooo, it is not easy to express the bounding boxes as strings in > > AVDictionaryEntry.value, the bounding box has several data members, > > and they are data and have high possibility to contain '\0' in the > > middle of the data. So, we might not use AVFrame.metadata. > > > > What your bounding box actually have? > > I thought it is just few numbers and string describing box, no? > > bounding box is the term that used by object detection papers for the objects detected in the picture, here is a visualized example at https://github.com/tensorflow/models/blob/master/research/object_detection/g3doc/img/kites_detections_output.jpg A frame can have multiple objects (or zero object), a bounding box is used to represent one detected object, including a rectangle, object label, and confidence for the detection. For each object, we can also apply 'classification filter', and so the bounding box also contains classification labels and classification confidences. See code at https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210301132053.30264-1-yejun.guo@intel.com/. It is possible that we present them in string, for example, "bbox0.top", "bbox1.detect_label" and "bbox2.classify_confidences1.den" etc. and I think the code is not elegant, and it is not close to our final solution (go into side data as structs).
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: 2021年3月4日 17:45 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > Quoting Guo, Yejun (2021-03-04 09:48:14) > > Hi, I think option 2 might be the good choice for now. > > Given past experience, I am very much not a fan of "for now" solutions. > They tend to ossify and remain in place for years (I have seen at least one > "//temporary hack" remain unchanged for over 10 years). Then further hacks > are grown on these "for now" solutions and they become very hard to change > or remove. > > So I would strongly prefer that you spend the effort now to design a proper > future-proof solution (most likely side data based). If it turns out to need some > change later, then there's plenty of precedent for changing API. > Agree, side data based solution is what we will finally use, and also my original code. There's no industry standard which defines the specification of bounding box. And I've tried my best for the design, investigated models, designed the structs and also refined it with feedback from community.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nicolas > George > Sent: 2021年3月4日 16:58 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: unref > private_ref when frame leaves libavfilter > > Guo, Yejun (12021-03-04): > > For option 1.1, it is not allowed because .h file (structs for > > bounding box) for side data is not exported. > > > > For option 1.2, to avoid the possible risk of API breaking, we need a > > versioning management, and the code such as BoundingBoxV1 and > > BoundingBoxV2 etc. is not elegant. > > Have you considered option 1.3: > > 1.3. Add the enum for a new side data, but document: > > /** > * The data is for internal use by libavfilter. > */ > > For the users, it is equivalent to storing it in private_ref, since it is private, but it > is closer to the good long-term API where the information is eventually > available outside. > Thanks a lot, I think this is the best option, will follow this if there is no objection.
On Thu, Mar 4, 2021 at 12:44 PM Guo, Yejun <yejun.guo@intel.com> wrote: > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Paul B > > Mahol > > Sent: 2021年3月4日 17:26 > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: > unref > > private_ref when frame leaves libavfilter > > > > On Mon, Mar 1, 2021 at 4:46 PM Guo, Yejun <yejun.guo@intel.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Nicolas > > > > George > > > > Sent: 2021年3月1日 23:07 > > > > To: FFmpeg development discussions and patches > > > > <ffmpeg-devel@ffmpeg.org> > > > > Subject: Re: [FFmpeg-devel] [PATCH V4 2/4] libavfilter/buffersink.c: > > > unref > > > > private_ref when frame leaves libavfilter > > > > > > > > Guo, Yejun (12021-03-01): > > > > > Actually, I think private_ref in libavfilter can only be used for > > > > > an exclusive usage at a time. > > > > > > > > Exactly. If we use it for this, then we cannot use for anything else > > > > in > > > libavfilter. > > > > This use seems too specific to warrant dedicating such an unique > > > > field > > > to it, > > > > even though we do not have a better use in sight. > > > > > > > > > As Paul mentioned, I think AVFrame.metadata is a better choice. > > > > > > > > If you can express it as a string or set of strings with a clear > > > > syntax > > > that can > > > > easily be parsed, then possibly, yes. > > > > > > ooo, it is not easy to express the bounding boxes as strings in > > > AVDictionaryEntry.value, the bounding box has several data members, > > > and they are data and have high possibility to contain '\0' in the > > > middle of the data. So, we might not use AVFrame.metadata. > > > > > > > What your bounding box actually have? > > > > I thought it is just few numbers and string describing box, no? > > > > > > bounding box is the term that used by object detection papers for > the objects detected in the picture, here is a visualized example at > > https://github.com/tensorflow/models/blob/master/research/object_detection/g3doc/img/kites_detections_output.jpg > > A frame can have multiple objects (or zero object), a bounding box > is used to represent one detected object, including a rectangle, object > label, and confidence for the detection. For each object, we can also > apply 'classification filter', and so the bounding box also contains > classification labels and classification confidences. See code at > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210301132053.30264-1-yejun.guo@intel.com/ > . > > It is possible that we present them in string, for example, "bbox0.top", > "bbox1.detect_label" and "bbox2.classify_confidences1.den" etc. > and I think the code is not elegant, and it is not close to our final > solution > (go into side data as structs). > You must use frame.metadata. Thus it will be available to all filters. > > > _______________________________________________ > 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".
Guo, Yejun (12021-03-04): > Agree, side data based solution is what we will finally use, and also > my original code. I think so too. > There's no industry standard which defines the specification of > bounding box. Mist industry standards suck anyway, it is best that we can define things properly exactly like we need. Regards,
diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c index 58848941d4..837a6e9e82 100644 --- a/libavfilter/buffersink.c +++ b/libavfilter/buffersink.c @@ -98,6 +98,7 @@ static int return_or_keep_frame(BufferSinkContext *buf, AVFrame *out, AVFrame *i } else { av_assert1(out); buf->peeked_frame = NULL; + av_buffer_unref(&in->private_ref); av_frame_move_ref(out, in); av_frame_free(&in); return 0;
private_ref is for internal use by a single libav* library. It has to be NULL when ownership of the frame leaves the respective library, buffersink is the last step when the frame leaves libavfilter, so add unref here. Signed-off-by: Guo, Yejun <yejun.guo@intel.com> --- libavfilter/buffersink.c | 1 + 1 file changed, 1 insertion(+)