| Message ID | 20171003131518.4557-3-nfxjfg@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > From: Anton Khirnov <anton@khirnov.net> > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > wrapped in the lavc struct and then unwrapped before the frame is > returned to the caller. this is a ugly hack one and the same field should not be used to hold both the users opaque_ref as well as a structure which is itself not a user opaque_ref [...]
On Tue, 3 Oct 2017 21:40:58 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > From: Anton Khirnov <anton@khirnov.net> > > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > wrapped in the lavc struct and then unwrapped before the frame is > > returned to the caller. > > this is a ugly hack > > one and the same field should not be used to hold both the > users opaque_ref as well as a structure which is itself not a user > opaque_ref While the AVFrame is within libavcodec, it's libavcodec's frame, not the user's. Thus your claim doesn't make too much sense. libavcodec fully controls the meaning for its own AVFrames' opaque_ref, but reconstruct the user's value when returning it. Though to be honest, I'd just have defined that get_buffer does not allow the field to be set.
On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > On Tue, 3 Oct 2017 21:40:58 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > > From: Anton Khirnov <anton@khirnov.net> > > > > > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > > wrapped in the lavc struct and then unwrapped before the frame is > > > returned to the caller. > > > > this is a ugly hack > > > > one and the same field should not be used to hold both the > > users opaque_ref as well as a structure which is itself not a user > > opaque_ref > > While the AVFrame is within libavcodec, it's libavcodec's frame, not > the user's. Thus your claim doesn't make too much sense. libavcodec > fully controls the meaning for its own AVFrames' opaque_ref, but > reconstruct the user's value when returning it. i disagree such hacks should not be added, we do have enough hacks already AVFrames are not really seperated into isolated classes There arent "the users AVFrames" vs. "the internal AVFrames" its fragile to create and maintain such seperation with interfaces that all wrap and unwrap the opaque_ref. Any mistake being a potential security issue and or crash Its MUCH more robust and also easier to understand to use a sperate field more so, opaque_ref is used in only 5 lines in the whole codebase, so there is not much code to consider when using a different solution [...]
On Wed, 4 Oct 2017 11:22:37 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > > On Tue, 3 Oct 2017 21:40:58 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > > > From: Anton Khirnov <anton@khirnov.net> > > > > > > > > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > > > wrapped in the lavc struct and then unwrapped before the frame is > > > > returned to the caller. > > > > > > this is a ugly hack > > > > > > one and the same field should not be used to hold both the > > > users opaque_ref as well as a structure which is itself not a user > > > opaque_ref > > > > While the AVFrame is within libavcodec, it's libavcodec's frame, not > > the user's. Thus your claim doesn't make too much sense. libavcodec > > fully controls the meaning for its own AVFrames' opaque_ref, but > > reconstruct the user's value when returning it. > > i disagree Well, you're wrong anyway. > such hacks should not be added, we do have enough hacks already It's not a hack. > AVFrames are not really seperated into isolated classes > There arent "the users AVFrames" vs. "the internal AVFrames" Oh yes, there is the concept of an "owner" of an AVFrame, and the owner of the AVFrame decides what opaque_ref means. It's quite literally for free use by the owner of the reference. That the code goes through some acrobatics to preserve opaque_ref as passed in by get_buffer is just a feature. > its fragile to create and maintain such seperation with interfaces > that all wrap and unwrap the opaque_ref. Any mistake being a potential > security issue and or crash This is done strictly when returning a valid AVFrame, so there is no room for mistakes. > Its MUCH more robust and also easier to understand to use a sperate > field No, it's not. If you fail to do call post_process() on returned AVFrames (which is done by the same code which exchanges opaque_ref) you have bugs that violate the API or crash anyway. > more so, opaque_ref is used in only 5 lines in the whole codebase, > so there is not much code to consider when using a different solution We shouldn't add such special fields, we have enough hacks already. Is that your only suggestion how to do this? Because it's a bad one.
On Wed, Oct 04, 2017 at 11:34:18AM +0200, wm4 wrote: > On Wed, 4 Oct 2017 11:22:37 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > > > On Tue, 3 Oct 2017 21:40:58 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > > > > From: Anton Khirnov <anton@khirnov.net> > > > > > > > > > > > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > > > > wrapped in the lavc struct and then unwrapped before the frame is > > > > > returned to the caller. > > > > > > > > this is a ugly hack > > > > > > > > one and the same field should not be used to hold both the > > > > users opaque_ref as well as a structure which is itself not a user > > > > opaque_ref > > > > > > While the AVFrame is within libavcodec, it's libavcodec's frame, not > > > the user's. Thus your claim doesn't make too much sense. libavcodec > > > fully controls the meaning for its own AVFrames' opaque_ref, but > > > reconstruct the user's value when returning it. > > > > i disagree > > Well, you're wrong anyway. > > > such hacks should not be added, we do have enough hacks already > > It's not a hack. please accept my veto to this patch. also, please stop sending me insults. * Loaded log from Sun Feb 26 13:41:05 2017 11:34 <wm4> I don't want to hear this from the king of ugly hacks thanks [...]
On Wed, 4 Oct 2017 11:47:39 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Oct 04, 2017 at 11:34:18AM +0200, wm4 wrote: > > On Wed, 4 Oct 2017 11:22:37 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > > > > On Tue, 3 Oct 2017 21:40:58 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > > > > > From: Anton Khirnov <anton@khirnov.net> > > > > > > > > > > > > > > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > > > > > wrapped in the lavc struct and then unwrapped before the frame is > > > > > > returned to the caller. > > > > > > > > > > this is a ugly hack > > > > > > > > > > one and the same field should not be used to hold both the > > > > > users opaque_ref as well as a structure which is itself not a user > > > > > opaque_ref > > > > > > > > While the AVFrame is within libavcodec, it's libavcodec's frame, not > > > > the user's. Thus your claim doesn't make too much sense. libavcodec > > > > fully controls the meaning for its own AVFrames' opaque_ref, but > > > > reconstruct the user's value when returning it. > > > > > > i disagree > > > > Well, you're wrong anyway. > > > > > such hacks should not be added, we do have enough hacks already > > > > It's not a hack. > > please accept my veto to this patch. That's completely unreasonable. You didn't even respond to my arguments. I can't accept your veto. > also, please stop sending me insults. > > * Loaded log from Sun Feb 26 13:41:05 2017 > 11:34 <wm4> I don't want to hear this from the king of ugly hacks I have the feeling your objections to this patch were bullshit in the first place, so I overreacted a little. But this "insult" wasn't meant to be public, so this is up to you. Anyway, you certainly never had problems with adding ugly hacks. It's possibly that you don't even deny this in most cases, but that you felt they were absolute "necessary" still.
On 04.10.2017 11:34, wm4 wrote: > On Wed, 4 Oct 2017 11:22:37 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > >> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: >>> On Tue, 3 Oct 2017 21:40:58 +0200 >>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: >>>>> From: Anton Khirnov <anton@khirnov.net> >>>>> >>>> >>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is >>>>> wrapped in the lavc struct and then unwrapped before the frame is >>>>> returned to the caller. >>>> >>>> this is a ugly hack >>>> >>>> one and the same field should not be used to hold both the >>>> users opaque_ref as well as a structure which is itself not a user >>>> opaque_ref >>> >>> While the AVFrame is within libavcodec, it's libavcodec's frame, not >>> the user's. Thus your claim doesn't make too much sense. libavcodec >>> fully controls the meaning for its own AVFrames' opaque_ref, but >>> reconstruct the user's value when returning it. >> >> i disagree > > Well, you're wrong anyway. > >> such hacks should not be added, we do have enough hacks already > > It's not a hack. Changing the semantics of a field during its lifetime, even when only done within private code, is at least unexpected behavior. >> AVFrames are not really seperated into isolated classes >> There arent "the users AVFrames" vs. "the internal AVFrames" > > Oh yes, there is the concept of an "owner" of an AVFrame, and the owner > of the AVFrame decides what opaque_ref means. It's quite literally for > free use by the owner of the reference. > > That the code goes through some acrobatics to preserve opaque_ref as > passed in by get_buffer is just a feature. > >> its fragile to create and maintain such seperation with interfaces >> that all wrap and unwrap the opaque_ref. Any mistake being a potential >> security issue and or crash > > This is done strictly when returning a valid AVFrame, so there is no > room for mistakes. The room for mistake might not increase for external developers but it increases for internal developers (maintenance cost). >> Its MUCH more robust and also easier to understand to use a sperate >> field > > No, it's not. If you fail to do call post_process() on returned > AVFrames (which is done by the same code which exchanges opaque_ref) > you have bugs that violate the API or crash anyway. > >> more so, opaque_ref is used in only 5 lines in the whole codebase, >> so there is not much code to consider when using a different solution > > We shouldn't add such special fields, we have enough hacks already. Is > that your only suggestion how to do this? Because it's a bad one. What would be the drawback of using a separate field? And please try to reduce the amount of emotions in this thread, and increase the amount of objective reasons. Regards, Tobias
On Wed, 4 Oct 2017 13:37:31 +0200 Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 04.10.2017 11:34, wm4 wrote: > > On Wed, 4 Oct 2017 11:22:37 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > >> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > >>> On Tue, 3 Oct 2017 21:40:58 +0200 > >>> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>> > >>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > >>>>> From: Anton Khirnov <anton@khirnov.net> > >>>>> > >>>> > >>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > >>>>> wrapped in the lavc struct and then unwrapped before the frame is > >>>>> returned to the caller. > >>>> > >>>> this is a ugly hack > >>>> > >>>> one and the same field should not be used to hold both the > >>>> users opaque_ref as well as a structure which is itself not a user > >>>> opaque_ref > >>> > >>> While the AVFrame is within libavcodec, it's libavcodec's frame, not > >>> the user's. Thus your claim doesn't make too much sense. libavcodec > >>> fully controls the meaning for its own AVFrames' opaque_ref, but > >>> reconstruct the user's value when returning it. > >> > >> i disagree > > > > Well, you're wrong anyway. > > > >> such hacks should not be added, we do have enough hacks already > > > > It's not a hack. > > Changing the semantics of a field during its lifetime, even when only > done within private code, is at least unexpected behavior. That's not done. Conceptually the AVFrame with the changed behavior is a new reference. Internally, AVFrame.opaque_ref will always have the same semantics, pointing to the FrameDecodeData struct. Only at points where the AVFrame ref is converted to/from the user struct this is changed. > > This is done strictly when returning a valid AVFrame, so there is no > > room for mistakes. > > The room for mistake might not increase for external developers but it > increases for internal developers (maintenance cost). Like where? There are only 2 places where the code needs to deal with it, and these are in shared code, not individual decoders. There are other cases where this will be needed, like fixing videotoolbox with frame threading. > >> Its MUCH more robust and also easier to understand to use a sperate > >> field > > > > No, it's not. If you fail to do call post_process() on returned > > AVFrames (which is done by the same code which exchanges opaque_ref) > > you have bugs that violate the API or crash anyway. > > > >> more so, opaque_ref is used in only 5 lines in the whole codebase, > >> so there is not much code to consider when using a different solution > > > > We shouldn't add such special fields, we have enough hacks already. Is > > that your only suggestion how to do this? Because it's a bad one. > > What would be the drawback of using a separate field? That would add a field that is merely needed for an implementation detail within libavcodec. Would you argue adding one for each case we encounter? Can user applications request the addition of arbitrary fields as well, which they might need internally? opaque_ref is basically a priv pointer, which _always_ has different meanings depending on the context where it's used. For example, what does AVCodecContext.priv_data point to? Or AVCodecContext.opaque? It's even worse with av_log(), which gets a void*, but somehow expects it to be "something" (a struct where the first member is AVClass* or so). So don't pretend this commits adds anything confusing if you're fine with the av_log() shit, or the way how most codecs/filters/(de)muxers are implemented. In any case, feel free to make a better suggestion for implementing this, which doesn't require adding pointless fields to AVFrame. This is just a Libav merge too - if you care about what gets merged, discuss it with the original author on the libav-devel mailing list before it gets committed. I shouldn't have to do this discussion in the author's place. (In fact I had a similar discussion with the author.)
2017-10-04 14:04 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > if you care about what gets merged, discuss it with the > original author on the libav-devel mailing list before it > gets committed. You do realize that you are asking for something that is not possible - did you forget? Carl Eugen
On Thu, 5 Oct 2017 09:02:10 +0200 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-04 14:04 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > > > if you care about what gets merged, discuss it with the > > original author on the libav-devel mailing list before it > > gets committed. > > You do realize that you are asking for something that > is not possible - did you forget? Yes, I forgot that it's impossible for you to communicate like a normal human being. Sorry I can't deal with all your bullshit all at once.
On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: > On Wed, 4 Oct 2017 13:37:31 +0200 > Tobias Rapp <t.rapp@noa-archive.com> wrote: > > > On 04.10.2017 11:34, wm4 wrote: > > > On Wed, 4 Oct 2017 11:22:37 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > >> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > > >>> On Tue, 3 Oct 2017 21:40:58 +0200 > > >>> Michael Niedermayer <michael@niedermayer.cc> wrote: > > >>> > > >>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > >>>>> From: Anton Khirnov <anton@khirnov.net> > > >>>>> > > >>>> > > >>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > >>>>> wrapped in the lavc struct and then unwrapped before the frame is > > >>>>> returned to the caller. > > >>>> > > >>>> this is a ugly hack > > >>>> > > >>>> one and the same field should not be used to hold both the > > >>>> users opaque_ref as well as a structure which is itself not a user > > >>>> opaque_ref > > >>> > > >>> While the AVFrame is within libavcodec, it's libavcodec's frame, not > > >>> the user's. Thus your claim doesn't make too much sense. libavcodec > > >>> fully controls the meaning for its own AVFrames' opaque_ref, but > > >>> reconstruct the user's value when returning it. > > >> > > >> i disagree > > > > > > Well, you're wrong anyway. > > > > > >> such hacks should not be added, we do have enough hacks already > > > > > > It's not a hack. > > > > Changing the semantics of a field during its lifetime, even when only > > done within private code, is at least unexpected behavior. > > That's not done. The semantics are defined by the docs, which state: "AVBufferRef for free use by the API user." And before the patch this is true, all instances of this field are controled by the user application and are consistent. after the patch the AVFrames used by a codec have their opaque_ref replaced by a wraped structure relative to what the outside of this codec has. > Conceptually the AVFrame with the changed behavior is > a new reference. Internally, AVFrame.opaque_ref will always have the > same semantics, pointing to the FrameDecodeData struct. Only at points > where the AVFrame ref is converted to/from the user struct this is > changed. > > > > This is done strictly when returning a valid AVFrame, so there is no > > > room for mistakes. > > > > The room for mistake might not increase for external developers but it > > increases for internal developers (maintenance cost). > > Like where? There are only 2 places where the code needs to deal with > it, and these are in shared code, not individual decoders. just greping for AVFrame in the headers shows callbacks, a direct pointer to a AVFrame and the API functions that interface the codec Just thinking of a codec that instanciates another codec and how exactly the callback which may originate from the user or the outer codec would unwrap the potential nested wraping. I really dont think we want this in FFmpeg And this is just one example ... [...]
On Thu, 5 Oct 2017 18:47:01 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: > > On Wed, 4 Oct 2017 13:37:31 +0200 > > Tobias Rapp <t.rapp@noa-archive.com> wrote: > > > > > On 04.10.2017 11:34, wm4 wrote: > > > > On Wed, 4 Oct 2017 11:22:37 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > >> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > > > >>> On Tue, 3 Oct 2017 21:40:58 +0200 > > > >>> Michael Niedermayer <michael@niedermayer.cc> wrote: > > > >>> > > > >>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > > > >>>>> From: Anton Khirnov <anton@khirnov.net> > > > >>>>> > > > >>>> > > > >>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > > >>>>> wrapped in the lavc struct and then unwrapped before the frame is > > > >>>>> returned to the caller. > > > >>>> > > > >>>> this is a ugly hack > > > >>>> > > > >>>> one and the same field should not be used to hold both the > > > >>>> users opaque_ref as well as a structure which is itself not a user > > > >>>> opaque_ref > > > >>> > > > >>> While the AVFrame is within libavcodec, it's libavcodec's frame, not > > > >>> the user's. Thus your claim doesn't make too much sense. libavcodec > > > >>> fully controls the meaning for its own AVFrames' opaque_ref, but > > > >>> reconstruct the user's value when returning it. > > > >> > > > >> i disagree > > > > > > > > Well, you're wrong anyway. > > > > > > > >> such hacks should not be added, we do have enough hacks already > > > > > > > > It's not a hack. > > > > > > Changing the semantics of a field during its lifetime, even when only > > > done within private code, is at least unexpected behavior. > > > > That's not done. > > The semantics are defined by the docs, which state: > "AVBufferRef for free use by the API user." libavcodec is the API user. I know the semantics of opaque_ref very well btw., because I added it. > And before the patch this is true, all instances of this field are > controled by the user application and are consistent. The "user" is the AVFrame user, which includes libavcodec. > after the patch the AVFrames used by a codec have their opaque_ref > replaced by a wraped structure relative to what the outside of this > codec has. > > > > Conceptually the AVFrame with the changed behavior is > > a new reference. Internally, AVFrame.opaque_ref will always have the > > same semantics, pointing to the FrameDecodeData struct. Only at points > > where the AVFrame ref is converted to/from the user struct this is > > changed. > > > > > > This is done strictly when returning a valid AVFrame, so there is no > > > > room for mistakes. > > > > > > The room for mistake might not increase for external developers but it > > > increases for internal developers (maintenance cost). > > > > Like where? There are only 2 places where the code needs to deal with > > it, and these are in shared code, not individual decoders. > > just greping for AVFrame in the headers shows callbacks, a direct > pointer to a AVFrame and the API functions that interface the codec Do you mean get_buffer? I explained that in-depth. > Just thinking of a codec that instanciates another codec and how > exactly the callback which may originate from the user or the outer > codec would unwrap the potential nested wraping. That would happen only if you'd nest get_buffer in an incorrect way. "Nested codecs" are an ugly hack anyway that are likely to break other things, especially if get_buffer is used. I see two decoders which recursively open other codecs (weird jpg variants). Both would not be broken by this patch, because they don't proxy get_buffer to the underlying codecs. (But what I see is that they require shitty ugly hacks due to shitty ugly codec locking, which was added as shitty ugly hack for shitty ugly broken shit. But apparently accepting that kind of code in FFmpeg is strangely no problem.) > I really dont think we want this in FFmpeg Why so obstinate. > And this is just one example ... Your one example was invalid, next one please.
2017-10-05 21:22 GMT+02:00 wm4 <nfxjfg@googlemail.com>: >> And this is just one example ... > > Your one example was invalid, next one please. One example is more than enough, we can all see that this is not clean new code. What is wrong about adding a new field if you really need it? Carl Eugen
On 05/10/17 17:47, Michael Niedermayer wrote: > On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: >> On Wed, 4 Oct 2017 13:37:31 +0200 >> Tobias Rapp <t.rapp@noa-archive.com> wrote: >> >>> On 04.10.2017 11:34, wm4 wrote: >>>> On Wed, 4 Oct 2017 11:22:37 +0200 >>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>> >>>>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: >>>>>> On Tue, 3 Oct 2017 21:40:58 +0200 >>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>> >>>>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: >>>>>>>> From: Anton Khirnov <anton@khirnov.net> >>>>>>>> >>>>>>> >>>>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is >>>>>>>> wrapped in the lavc struct and then unwrapped before the frame is >>>>>>>> returned to the caller. >>>>>>> >>>>>>> this is a ugly hack >>>>>>> >>>>>>> one and the same field should not be used to hold both the >>>>>>> users opaque_ref as well as a structure which is itself not a user >>>>>>> opaque_ref >>>>>> >>>>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not >>>>>> the user's. Thus your claim doesn't make too much sense. libavcodec >>>>>> fully controls the meaning for its own AVFrames' opaque_ref, but >>>>>> reconstruct the user's value when returning it. >>>>> >>>>> i disagree >>>> >>>> Well, you're wrong anyway. >>>> >>>>> such hacks should not be added, we do have enough hacks already >>>> >>>> It's not a hack. >>> >>> Changing the semantics of a field during its lifetime, even when only >>> done within private code, is at least unexpected behavior. >> >> That's not done. > > The semantics are defined by the docs, which state: > "AVBufferRef for free use by the API user." > > And before the patch this is true, all instances of this field are > controled by the user application and are consistent. > > after the patch the AVFrames used by a codec have their opaque_ref > replaced by a wraped structure relative to what the outside of this > codec has. > > >> Conceptually the AVFrame with the changed behavior is >> a new reference. Internally, AVFrame.opaque_ref will always have the >> same semantics, pointing to the FrameDecodeData struct. Only at points >> where the AVFrame ref is converted to/from the user struct this is >> changed. >> >>>> This is done strictly when returning a valid AVFrame, so there is no >>>> room for mistakes. >>> >>> The room for mistake might not increase for external developers but it >>> increases for internal developers (maintenance cost). >> >> Like where? There are only 2 places where the code needs to deal with >> it, and these are in shared code, not individual decoders. > > just greping for AVFrame in the headers shows callbacks, a direct > pointer to a AVFrame and the API functions that interface the codec > > Just thinking of a codec that instanciates another codec and how > exactly the callback which may originate from the user or the outer > codec would unwrap the potential nested wraping. > I really dont think we want this in FFmpeg > And this is just one example ... I don't understand this discussion. As far as I can tell, the sequence is this: * libavcodec allocates an AVFrame structure. * libavcodec calls get_buffer2 with that AVFrame structure; the user fills its fields. * libavcodec extracts the buffer references and metadata from the AVFrame, and maybe frees it (some codecs reuse a single AVFrame for the lifetime of the codec, others allocate them each time). ~ decoding happens, the buffers are written to ~ * The user allocates a new AVFrame structure. * The user calls receive_frame with that new AVFrame structure; libavcodec its fields with references to the decoded data and associated metadata. * The user can then read the buffers of the frame, along with its metadata. Why would it matter what happens in the middle? The AVFrame structure at the end is not the AVFrame structure at the start, and the user can't assume anything about it at all - if they try to dereference a pointer to the AVFrame supplied by libavcodec for get_buffer2 after get_buffer2 has returned then they are definitely invoking undefined behaviour. - Mark
On Thu, 5 Oct 2017 22:01:56 +0200 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-05 21:22 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > > >> And this is just one example ... > > > > Your one example was invalid, next one please. > > One example is more than enough, we can all see that this > is not clean new code. But I showed that his example isn't actually a problem. > What is wrong about adding a new field if you really need it? Because AVFrame shouldn't have internal libavcodec fields?
On Thu, Oct 05, 2017 at 09:03:40PM +0100, Mark Thompson wrote: > On 05/10/17 17:47, Michael Niedermayer wrote: > > On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: > >> On Wed, 4 Oct 2017 13:37:31 +0200 > >> Tobias Rapp <t.rapp@noa-archive.com> wrote: > >> > >>> On 04.10.2017 11:34, wm4 wrote: > >>>> On Wed, 4 Oct 2017 11:22:37 +0200 > >>>> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>>> > >>>>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > >>>>>> On Tue, 3 Oct 2017 21:40:58 +0200 > >>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>>>>> > >>>>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > >>>>>>>> From: Anton Khirnov <anton@khirnov.net> > >>>>>>>> > >>>>>>> > >>>>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > >>>>>>>> wrapped in the lavc struct and then unwrapped before the frame is > >>>>>>>> returned to the caller. > >>>>>>> > >>>>>>> this is a ugly hack > >>>>>>> > >>>>>>> one and the same field should not be used to hold both the > >>>>>>> users opaque_ref as well as a structure which is itself not a user > >>>>>>> opaque_ref > >>>>>> > >>>>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not > >>>>>> the user's. Thus your claim doesn't make too much sense. libavcodec > >>>>>> fully controls the meaning for its own AVFrames' opaque_ref, but > >>>>>> reconstruct the user's value when returning it. > >>>>> > >>>>> i disagree > >>>> > >>>> Well, you're wrong anyway. > >>>> > >>>>> such hacks should not be added, we do have enough hacks already > >>>> > >>>> It's not a hack. > >>> > >>> Changing the semantics of a field during its lifetime, even when only > >>> done within private code, is at least unexpected behavior. > >> > >> That's not done. > > > > The semantics are defined by the docs, which state: > > "AVBufferRef for free use by the API user." > > > > And before the patch this is true, all instances of this field are > > controled by the user application and are consistent. > > > > after the patch the AVFrames used by a codec have their opaque_ref > > replaced by a wraped structure relative to what the outside of this > > codec has. > > > > > >> Conceptually the AVFrame with the changed behavior is > >> a new reference. Internally, AVFrame.opaque_ref will always have the > >> same semantics, pointing to the FrameDecodeData struct. Only at points > >> where the AVFrame ref is converted to/from the user struct this is > >> changed. > >> > >>>> This is done strictly when returning a valid AVFrame, so there is no > >>>> room for mistakes. > >>> > >>> The room for mistake might not increase for external developers but it > >>> increases for internal developers (maintenance cost). > >> > >> Like where? There are only 2 places where the code needs to deal with > >> it, and these are in shared code, not individual decoders. > > > > just greping for AVFrame in the headers shows callbacks, a direct > > pointer to a AVFrame and the API functions that interface the codec > > > > Just thinking of a codec that instanciates another codec and how > > exactly the callback which may originate from the user or the outer > > codec would unwrap the potential nested wraping. > > I really dont think we want this in FFmpeg > > And this is just one example ... > > I don't understand this discussion. yes, i realize this, but iam not sure why its pretty simple and clear but you seem to skip over parts of what i said. > > As far as I can tell, the sequence is this: > > * libavcodec allocates an AVFrame structure. > * libavcodec calls get_buffer2 with that AVFrame structure; the user fills its fields. > * libavcodec extracts the buffer references and metadata from the AVFrame, and maybe frees it (some codecs reuse a single AVFrame for the lifetime of the codec, others allocate them each time). > ~ decoding happens, the buffers are written to ~ > * The user allocates a new AVFrame structure. > * The user calls receive_frame with that new AVFrame structure; libavcodec its fields with references to the decoded data and associated metadata. > * The user can then read the buffers of the frame, along with its metadata. > > Why would it matter what happens in the middle? The AVFrame structure at the end is not the AVFrame structure at the start, and the user can't assume anything about it at all - if they try to dereference a pointer to the AVFrame supplied by libavcodec for get_buffer2 after get_buffer2 has returned then they are definitely invoking undefined behaviour. First this does not consider nested codecs. A decoder can instanciate a internal decoder, if that now uses the user callback it would have to unwrap twice for it if it used a callback from the outer decoder it has to unwrap once. This of course also depends on how it instanciates the decoder, that is at which API level. Next, if you look at the API and search for AVFrame you will find that there are more callbacks than get_buffer2() that use AVFrames. there is for example also void (*draw_horiz_band)(struct AVCodecContext *s, const AVFrame *src, int offset[AV_NUM_DATA_POINTERS], int y, int type, int height); Which too has a AVFrame, which too must be unwraped and in a way specific on the nesting depth of the decoder. and then there is AVFrame *coded_frame; in AVCodecContext Which can be accessed by the user as well as codec. Iam pretty sure if i look more ill find more examples. And future API changes would all have to consider this wraping and unwraping. And then all above is just about decoders, there are encoders too some share code with decoders and at the same time encoders take AVFrames from the user. Does the shared code expect wraped opaque_ref or not? All this adds alot of complexity to maintaining the code and it would add many bugs The opaque_ref wraping is a really bad design. Iam not sure why people defend it. [...]
On 05/10/17 23:01, Michael Niedermayer wrote: > On Thu, Oct 05, 2017 at 09:03:40PM +0100, Mark Thompson wrote: >> On 05/10/17 17:47, Michael Niedermayer wrote: >>> On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: >>>> On Wed, 4 Oct 2017 13:37:31 +0200 >>>> Tobias Rapp <t.rapp@noa-archive.com> wrote: >>>> >>>>> On 04.10.2017 11:34, wm4 wrote: >>>>>> On Wed, 4 Oct 2017 11:22:37 +0200 >>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>> >>>>>>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: >>>>>>>> On Tue, 3 Oct 2017 21:40:58 +0200 >>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>>>> >>>>>>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: >>>>>>>>>> From: Anton Khirnov <anton@khirnov.net> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is >>>>>>>>>> wrapped in the lavc struct and then unwrapped before the frame is >>>>>>>>>> returned to the caller. >>>>>>>>> >>>>>>>>> this is a ugly hack >>>>>>>>> >>>>>>>>> one and the same field should not be used to hold both the >>>>>>>>> users opaque_ref as well as a structure which is itself not a user >>>>>>>>> opaque_ref >>>>>>>> >>>>>>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not >>>>>>>> the user's. Thus your claim doesn't make too much sense. libavcodec >>>>>>>> fully controls the meaning for its own AVFrames' opaque_ref, but >>>>>>>> reconstruct the user's value when returning it. >>>>>>> >>>>>>> i disagree >>>>>> >>>>>> Well, you're wrong anyway. >>>>>> >>>>>>> such hacks should not be added, we do have enough hacks already >>>>>> >>>>>> It's not a hack. >>>>> >>>>> Changing the semantics of a field during its lifetime, even when only >>>>> done within private code, is at least unexpected behavior. >>>> >>>> That's not done. >>> >>> The semantics are defined by the docs, which state: >>> "AVBufferRef for free use by the API user." >>> >>> And before the patch this is true, all instances of this field are >>> controled by the user application and are consistent. >>> >>> after the patch the AVFrames used by a codec have their opaque_ref >>> replaced by a wraped structure relative to what the outside of this >>> codec has. >>> >>> >>>> Conceptually the AVFrame with the changed behavior is >>>> a new reference. Internally, AVFrame.opaque_ref will always have the >>>> same semantics, pointing to the FrameDecodeData struct. Only at points >>>> where the AVFrame ref is converted to/from the user struct this is >>>> changed. >>>> >>>>>> This is done strictly when returning a valid AVFrame, so there is no >>>>>> room for mistakes. >>>>> >>>>> The room for mistake might not increase for external developers but it >>>>> increases for internal developers (maintenance cost). >>>> >>>> Like where? There are only 2 places where the code needs to deal with >>>> it, and these are in shared code, not individual decoders. >>> >>> just greping for AVFrame in the headers shows callbacks, a direct >>> pointer to a AVFrame and the API functions that interface the codec >>> >>> Just thinking of a codec that instanciates another codec and how >>> exactly the callback which may originate from the user or the outer >>> codec would unwrap the potential nested wraping. >>> I really dont think we want this in FFmpeg >>> And this is just one example ... >> > >> I don't understand this discussion. > > yes, i realize this, but iam not sure why > > its pretty simple and clear but you seem to skip over parts of what > i said. > > >> >> As far as I can tell, the sequence is this: >> >> * libavcodec allocates an AVFrame structure. >> * libavcodec calls get_buffer2 with that AVFrame structure; the user fills its fields. >> * libavcodec extracts the buffer references and metadata from the AVFrame, and maybe frees it (some codecs reuse a single AVFrame for the lifetime of the codec, others allocate them each time). >> ~ decoding happens, the buffers are written to ~ >> * The user allocates a new AVFrame structure. >> * The user calls receive_frame with that new AVFrame structure; libavcodec its fields with references to the decoded data and associated metadata. >> * The user can then read the buffers of the frame, along with its metadata. >> >> Why would it matter what happens in the middle? The AVFrame structure at the end is not the AVFrame structure at the start, and the user can't assume anything about it at all - if they try to dereference a pointer to the AVFrame supplied by libavcodec for get_buffer2 after get_buffer2 has returned then they are definitely invoking undefined behaviour. > > First this does not consider nested codecs. > A decoder can instanciate a internal decoder, if that now uses the > user callback it would have to unwrap twice for it if it used a > callback from the outer decoder it has to unwrap once. > This of course also depends on how it instanciates the decoder, that > is at which API level. All decoders pass back to their callers the opaque reference they received, so I don't think nesting is relevant. > Next, if you look at the API and search for AVFrame you will find > that there are more callbacks than get_buffer2() that use AVFrames. > there is for example also > void (*draw_horiz_band)(struct AVCodecContext *s, > const AVFrame *src, int offset[AV_NUM_DATA_POINTERS], > int y, int type, int height); > > Which too has a AVFrame, which too must be unwraped and in a way specific > on the nesting depth of the decoder. Hmm, ok, I didn't know about this one, and it was never mentioned before. > and then there is > AVFrame *coded_frame; > in AVCodecContext > > Which can be accessed by the user as well as codec. And immediately above it: * - decoding: unused > Iam pretty sure if i look more ill find more examples.> > And future API changes would all have to consider this wraping and > unwraping. > > And then all above is just about decoders, there are encoders too > some share code with decoders and at the same time encoders take > AVFrames from the user. Does the shared code expect wraped opaque_ref > or not? > All this adds alot of complexity to maintaining the code and it would > add many bugs > > The opaque_ref wraping is a really bad design. Iam not sure why > people defend it. I disagree with your implication here. I was under the impression that decoders have exactly one entry and one exit point for frames (get_buffer2 and receive_frame): since the opaque reference is only considered at those two points, there is no additional complexity anywhere and no other code need care about it. However, apparently the API abstraction has additional holes in it which I was not aware of (the draw_horiz_band callback you have noted above), which seems to invalidate that argument. Are there any other holes in the decoding or encoding abstractions through which frames are able to leak? It would be helpful to have a list of these somewhere so that people considering how AVFrames travel through libavcodec don't get caught out by this sort of thing in future. Thanks, - Mark
On 05/10/17 23:59, Mark Thompson wrote: > On 05/10/17 23:01, Michael Niedermayer wrote: >> On Thu, Oct 05, 2017 at 09:03:40PM +0100, Mark Thompson wrote: >>> On 05/10/17 17:47, Michael Niedermayer wrote: >>>> On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: >>>>> On Wed, 4 Oct 2017 13:37:31 +0200 >>>>> Tobias Rapp <t.rapp@noa-archive.com> wrote: >>>>> >>>>>> On 04.10.2017 11:34, wm4 wrote: >>>>>>> On Wed, 4 Oct 2017 11:22:37 +0200 >>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>>> >>>>>>>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: >>>>>>>>> On Tue, 3 Oct 2017 21:40:58 +0200 >>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>>>>> >>>>>>>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: >>>>>>>>>>> From: Anton Khirnov <anton@khirnov.net> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is >>>>>>>>>>> wrapped in the lavc struct and then unwrapped before the frame is >>>>>>>>>>> returned to the caller. >>>>>>>>>> >>>>>>>>>> this is a ugly hack >>>>>>>>>> >>>>>>>>>> one and the same field should not be used to hold both the >>>>>>>>>> users opaque_ref as well as a structure which is itself not a user >>>>>>>>>> opaque_ref >>>>>>>>> >>>>>>>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not >>>>>>>>> the user's. Thus your claim doesn't make too much sense. libavcodec >>>>>>>>> fully controls the meaning for its own AVFrames' opaque_ref, but >>>>>>>>> reconstruct the user's value when returning it. >>>>>>>> >>>>>>>> i disagree >>>>>>> >>>>>>> Well, you're wrong anyway. >>>>>>> >>>>>>>> such hacks should not be added, we do have enough hacks already >>>>>>> >>>>>>> It's not a hack. >>>>>> >>>>>> Changing the semantics of a field during its lifetime, even when only >>>>>> done within private code, is at least unexpected behavior. >>>>> >>>>> That's not done. >>>> >>>> The semantics are defined by the docs, which state: >>>> "AVBufferRef for free use by the API user." >>>> >>>> And before the patch this is true, all instances of this field are >>>> controled by the user application and are consistent. >>>> >>>> after the patch the AVFrames used by a codec have their opaque_ref >>>> replaced by a wraped structure relative to what the outside of this >>>> codec has. >>>> >>>> >>>>> Conceptually the AVFrame with the changed behavior is >>>>> a new reference. Internally, AVFrame.opaque_ref will always have the >>>>> same semantics, pointing to the FrameDecodeData struct. Only at points >>>>> where the AVFrame ref is converted to/from the user struct this is >>>>> changed. >>>>> >>>>>>> This is done strictly when returning a valid AVFrame, so there is no >>>>>>> room for mistakes. >>>>>> >>>>>> The room for mistake might not increase for external developers but it >>>>>> increases for internal developers (maintenance cost). >>>>> >>>>> Like where? There are only 2 places where the code needs to deal with >>>>> it, and these are in shared code, not individual decoders. >>>> >>>> just greping for AVFrame in the headers shows callbacks, a direct >>>> pointer to a AVFrame and the API functions that interface the codec >>>> >>>> Just thinking of a codec that instanciates another codec and how >>>> exactly the callback which may originate from the user or the outer >>>> codec would unwrap the potential nested wraping. >>>> I really dont think we want this in FFmpeg >>>> And this is just one example ... >>> >> >>> I don't understand this discussion. >> >> yes, i realize this, but iam not sure why >> >> its pretty simple and clear but you seem to skip over parts of what >> i said. >> >> >>> >>> As far as I can tell, the sequence is this: >>> >>> * libavcodec allocates an AVFrame structure. >>> * libavcodec calls get_buffer2 with that AVFrame structure; the user fills its fields. >>> * libavcodec extracts the buffer references and metadata from the AVFrame, and maybe frees it (some codecs reuse a single AVFrame for the lifetime of the codec, others allocate them each time). >>> ~ decoding happens, the buffers are written to ~ >>> * The user allocates a new AVFrame structure. >>> * The user calls receive_frame with that new AVFrame structure; libavcodec its fields with references to the decoded data and associated metadata. >>> * The user can then read the buffers of the frame, along with its metadata. >>> >>> Why would it matter what happens in the middle? The AVFrame structure at the end is not the AVFrame structure at the start, and the user can't assume anything about it at all - if they try to dereference a pointer to the AVFrame supplied by libavcodec for get_buffer2 after get_buffer2 has returned then they are definitely invoking undefined behaviour. >> >> First this does not consider nested codecs. >> A decoder can instanciate a internal decoder, if that now uses the >> user callback it would have to unwrap twice for it if it used a >> callback from the outer decoder it has to unwrap once. >> This of course also depends on how it instanciates the decoder, that >> is at which API level. > > All decoders pass back to their callers the opaque reference they received, so I don't think nesting is relevant. > >> Next, if you look at the API and search for AVFrame you will find >> that there are more callbacks than get_buffer2() that use AVFrames. >> there is for example also >> void (*draw_horiz_band)(struct AVCodecContext *s, >> const AVFrame *src, int offset[AV_NUM_DATA_POINTERS], >> int y, int type, int height); >> >> Which too has a AVFrame, which too must be unwraped and in a way specific >> on the nesting depth of the decoder. > > Hmm, ok, I didn't know about this one, and it was never mentioned before. > >> and then there is >> AVFrame *coded_frame; >> in AVCodecContext >> >> Which can be accessed by the user as well as codec. > > And immediately above it: > > * - decoding: unused > >> Iam pretty sure if i look more ill find more examples.> >> And future API changes would all have to consider this wraping and >> unwraping. >> >> And then all above is just about decoders, there are encoders too >> some share code with decoders and at the same time encoders take >> AVFrames from the user. Does the shared code expect wraped opaque_ref >> or not? >> All this adds alot of complexity to maintaining the code and it would >> add many bugs >> >> The opaque_ref wraping is a really bad design. Iam not sure why >> people defend it. > > I disagree with your implication here. I was under the impression that decoders have exactly one entry and one exit point for frames (get_buffer2 and receive_frame): since the opaque reference is only considered at those two points, there is no additional complexity anywhere and no other code need care about it. However, apparently the API abstraction has additional holes in it which I was not aware of (the draw_horiz_band callback you have noted above), which seems to invalidate that argument. > > Are there any other holes in the decoding or encoding abstractions through which frames are able to leak? It would be helpful to have a list of these somewhere so that people considering how AVFrames travel through libavcodec don't get caught out by this sort of thing in future. Having thought about this a bit more, the wrapped_avframe decoder rather messes with it too. If the input frame (packet) has opaque_ref set then the unwrap process would try to use it and fail.
On Fri, 6 Oct 2017 00:01:30 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > The opaque_ref wraping is a really bad design. Iam not sure why > people defend it. FFmpeg is full of this design. There are plenty of structs with opaque/priv fields that change meaning depending on the context (basically how the struct is used or what uses it). It affects all decoders, encoders, filters, demuxers, muxers, the av_log() call, functions that work with AVClass, AVOption, and probably more. Are you saying that FFmpeg is really bad design? That's funny. There is nothing unclean about this use of opaque_ref, just that it somewhat collides with awful legacy hacks like draw_horiz_band (but which could be fixed anyway). Also what you said about nested decoders is simply incorrect. The nested decoder is used only by the decoder implementation itself, and always uses the default get_buffer callback for the nested codec, so the returned AVFrame.opaque_ref is returned as NULL. Now it's possible that the new code will assert if the parent codec has AV_CODEC_CAP_DR1 set. This is because if AV_CODEC_CAP_DR1 is set, the AVFrame is expected to be allocated by the codec context's ff_get_buffer() (which would guarantee opaque_ref is set correctly). It looks like avrndec.c does not do this correctly if is_mjpeg is set. But in fact, this codec violates the API by declaring DR1 support, but not calling the user get_buffer2 callback. This could be fixed. An actually valid point is brought up by Mark Thompson (and not by you), but this can be fixed easily by wrapping the AVFrame properly before returning. (Assuming the desired semantics are that the opaque_ref is returned to the user as-is.) So, still no problem found. What is the problem? Now I'm very curious to hear what your "cleaner" solution to this problem is.
On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > On Fri, 6 Oct 2017 00:01:30 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > The opaque_ref wraping is a really bad design. Iam not sure why > > people defend it. > > FFmpeg is full of this design. There are plenty of structs with > opaque/priv fields that change meaning depending on the context > (basically how the struct is used or what uses it). It affects all > decoders, encoders, filters, demuxers, muxers, the av_log() call, > functions that work with AVClass, AVOption, and probably more. what you write is not true each decoder, demuxer, ... CLASS has its own type of private context nothing outside code specific to that class messes with it. A snow decoder has a snow context. If the outside structure is moved around its still a snow decoder with a snow private context. No amount of moving the structure around makes it invalid. OTOH, opaque_ref is defined by the user application. There is a single user application in the address space. Before the patch all AVFrame opaque_ref have the same type, no amount of moving/passing AVFrames around results in an invalid AVFrame. after the patch this is very different. Each AVFrame becomes tied to the code that created it, opaque_ref has a type that outside decoders, is defined by the user application inside decoders, its a internal structure. AVFrames are no longer a universal structure that can simply be passed around. This is bad design and it is fragile private and opaque are also intended to be very different things. In FFmpeg private is a internal thing like the internal context of an encoder. Opaque is a exteral thing, from the user application or caller You may have misunderstood me but I am against AVFrames depending on the code that created them. This is not just about this implementation (which is in fact buggy too), Pointing to the implementation issues and bugs was just intended to show how fragile this is. I do not understand how one can on one hand see all the problems this causes yet not see that the API that causes this is what is at fault. And it is MUCH easier to fix the API than to fix all the issues that context specific AVFrames would cause > > Are you saying that FFmpeg is really bad design? That's funny. > > There is nothing unclean about this use of opaque_ref, just that it > somewhat collides with awful legacy hacks like draw_horiz_band (but > which could be fixed anyway). > > Also what you said about nested decoders is simply incorrect. The It is not incorrect in case current implementations of decoders dont trigger it. [...] > > Now I'm very curious to hear what your "cleaner" solution to this > problem is. add a new field to AVFrame if you want a field with semantics that are different. you said you are against this IIRC. But thats the simple, robust and easy maintainable solution more so if that field is not void* it could provide type checking which most people consider a good thing. A system simiar to side data for opaque data could be used too, i would say thats overkill but some people like side data One entry for the users opaque structure One entry for the libavcodec private structure One entry for a future libavfilter private structure And this way you need no wraping or unwraping, a AVFrame either has some extra opaque fields or it doesnt. nothing could cast them to the wrong type. This is much more robust than putting all these things in opaque_ref and spending time and more time and more time writing code wraping and unwraping at every interface point and then cursing half the interface and likely in the future fighting over every bit of added interface as it massivly increases complexity with the wraping [...]
On Fri, Oct 06, 2017 at 12:38:44AM +0100, Mark Thompson wrote: > On 05/10/17 23:59, Mark Thompson wrote: > > On 05/10/17 23:01, Michael Niedermayer wrote: > >> On Thu, Oct 05, 2017 at 09:03:40PM +0100, Mark Thompson wrote: > >>> On 05/10/17 17:47, Michael Niedermayer wrote: > >>>> On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote: > >>>>> On Wed, 4 Oct 2017 13:37:31 +0200 > >>>>> Tobias Rapp <t.rapp@noa-archive.com> wrote: > >>>>> > >>>>>> On 04.10.2017 11:34, wm4 wrote: > >>>>>>> On Wed, 4 Oct 2017 11:22:37 +0200 > >>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>>>>>> > >>>>>>>> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote: > >>>>>>>>> On Tue, 3 Oct 2017 21:40:58 +0200 > >>>>>>>>> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>>>>>>>> > >>>>>>>>>> On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote: > >>>>>>>>>>> From: Anton Khirnov <anton@khirnov.net> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > >>>>>>>>>>> wrapped in the lavc struct and then unwrapped before the frame is > >>>>>>>>>>> returned to the caller. > >>>>>>>>>> > >>>>>>>>>> this is a ugly hack > >>>>>>>>>> > >>>>>>>>>> one and the same field should not be used to hold both the > >>>>>>>>>> users opaque_ref as well as a structure which is itself not a user > >>>>>>>>>> opaque_ref > >>>>>>>>> > >>>>>>>>> While the AVFrame is within libavcodec, it's libavcodec's frame, not > >>>>>>>>> the user's. Thus your claim doesn't make too much sense. libavcodec > >>>>>>>>> fully controls the meaning for its own AVFrames' opaque_ref, but > >>>>>>>>> reconstruct the user's value when returning it. > >>>>>>>> > >>>>>>>> i disagree > >>>>>>> > >>>>>>> Well, you're wrong anyway. > >>>>>>> > >>>>>>>> such hacks should not be added, we do have enough hacks already > >>>>>>> > >>>>>>> It's not a hack. > >>>>>> > >>>>>> Changing the semantics of a field during its lifetime, even when only > >>>>>> done within private code, is at least unexpected behavior. > >>>>> > >>>>> That's not done. > >>>> > >>>> The semantics are defined by the docs, which state: > >>>> "AVBufferRef for free use by the API user." > >>>> > >>>> And before the patch this is true, all instances of this field are > >>>> controled by the user application and are consistent. > >>>> > >>>> after the patch the AVFrames used by a codec have their opaque_ref > >>>> replaced by a wraped structure relative to what the outside of this > >>>> codec has. > >>>> > >>>> > >>>>> Conceptually the AVFrame with the changed behavior is > >>>>> a new reference. Internally, AVFrame.opaque_ref will always have the > >>>>> same semantics, pointing to the FrameDecodeData struct. Only at points > >>>>> where the AVFrame ref is converted to/from the user struct this is > >>>>> changed. > >>>>> > >>>>>>> This is done strictly when returning a valid AVFrame, so there is no > >>>>>>> room for mistakes. > >>>>>> > >>>>>> The room for mistake might not increase for external developers but it > >>>>>> increases for internal developers (maintenance cost). > >>>>> > >>>>> Like where? There are only 2 places where the code needs to deal with > >>>>> it, and these are in shared code, not individual decoders. > >>>> > >>>> just greping for AVFrame in the headers shows callbacks, a direct > >>>> pointer to a AVFrame and the API functions that interface the codec > >>>> > >>>> Just thinking of a codec that instanciates another codec and how > >>>> exactly the callback which may originate from the user or the outer > >>>> codec would unwrap the potential nested wraping. > >>>> I really dont think we want this in FFmpeg > >>>> And this is just one example ... > >>> > >> > >>> I don't understand this discussion. > >> > >> yes, i realize this, but iam not sure why > >> > >> its pretty simple and clear but you seem to skip over parts of what > >> i said. > >> > >> > >>> > >>> As far as I can tell, the sequence is this: > >>> > >>> * libavcodec allocates an AVFrame structure. > >>> * libavcodec calls get_buffer2 with that AVFrame structure; the user fills its fields. > >>> * libavcodec extracts the buffer references and metadata from the AVFrame, and maybe frees it (some codecs reuse a single AVFrame for the lifetime of the codec, others allocate them each time). > >>> ~ decoding happens, the buffers are written to ~ > >>> * The user allocates a new AVFrame structure. > >>> * The user calls receive_frame with that new AVFrame structure; libavcodec its fields with references to the decoded data and associated metadata. > >>> * The user can then read the buffers of the frame, along with its metadata. > >>> > >>> Why would it matter what happens in the middle? The AVFrame structure at the end is not the AVFrame structure at the start, and the user can't assume anything about it at all - if they try to dereference a pointer to the AVFrame supplied by libavcodec for get_buffer2 after get_buffer2 has returned then they are definitely invoking undefined behaviour. > >> > >> First this does not consider nested codecs. > >> A decoder can instanciate a internal decoder, if that now uses the > >> user callback it would have to unwrap twice for it if it used a > >> callback from the outer decoder it has to unwrap once. > >> This of course also depends on how it instanciates the decoder, that > >> is at which API level. > > > > All decoders pass back to their callers the opaque reference they received, so I don't think nesting is relevant. > > > >> Next, if you look at the API and search for AVFrame you will find > >> that there are more callbacks than get_buffer2() that use AVFrames. > >> there is for example also > >> void (*draw_horiz_band)(struct AVCodecContext *s, > >> const AVFrame *src, int offset[AV_NUM_DATA_POINTERS], > >> int y, int type, int height); > >> > >> Which too has a AVFrame, which too must be unwraped and in a way specific > >> on the nesting depth of the decoder. > > > > Hmm, ok, I didn't know about this one, and it was never mentioned before. > > > >> and then there is > >> AVFrame *coded_frame; > >> in AVCodecContext > >> > >> Which can be accessed by the user as well as codec. > > > > And immediately above it: > > > > * - decoding: unused > > > >> Iam pretty sure if i look more ill find more examples.> > >> And future API changes would all have to consider this wraping and > >> unwraping. > >> > >> And then all above is just about decoders, there are encoders too > >> some share code with decoders and at the same time encoders take > >> AVFrames from the user. Does the shared code expect wraped opaque_ref > >> or not? > >> All this adds alot of complexity to maintaining the code and it would > >> add many bugs > >> > >> The opaque_ref wraping is a really bad design. Iam not sure why > >> people defend it. > > > > I disagree with your implication here. I was under the impression that decoders have exactly one entry and one exit point for frames (get_buffer2 and receive_frame): since the opaque reference is only considered at those two points, there is no additional complexity anywhere and no other code need care about it. However, apparently the API abstraction has additional holes in it which I was not aware of (the draw_horiz_band callback you have noted above), which seems to invalidate that argument. > > > > Are there any other holes in the decoding or encoding abstractions through which frames are able to leak? It would be helpful to have a list of these somewhere so that people considering how AVFrames travel through libavcodec don't get caught out by this sort of thing in future. I dont really know and as a maintainer of some of this code, i dont really want to have to keep track of how exactly AVFrames can pass through the code. And as the one taking care of a lot of security i totally have to reject this. It turns a robust system into something thats really fragile. This is only secure if every way a AVFrame can pass through the code has each interface point carefully convert the opaque_ref type. Thats a large source of bugs and exploits. Just look at how hard it is to get this patch work initially. How many future commits will change the way AVFrames move and incorrectly update all the wraping. developers are not machines this suprising requirement of (un)wraping on interfaces will not be done correctly in a few years from now even if you get it all correct now. Now add libavformat, libavdevice, libavfilter specific opaque_ref wraping you really consider this to be good design by any definition of "good"? > > Having thought about this a bit more, the wrapped_avframe decoder rather messes with it too. If the input frame (packet) has opaque_ref set then the unwrap process would try to use it and fail. [...]
On Fri, 13 Oct 2017 20:03:12 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > I dont really know and as a maintainer of some of this code, i dont > really want to have to keep track of how exactly AVFrames can pass > through the code. You have to do that anyway. > And as the one taking care of a lot of security i totally have to > reject this. It turns a robust system into something thats really > fragile. This is only secure if every way a AVFrame can pass through > the code has each interface point carefully convert the opaque_ref type. > Thats a large source of bugs and exploits. The security argument is bullshit. It's very clearly defined how the field behaves. If you pass it to libavcodec you have to wrap it, if you return it you have to unwrap it. > Just look at how hard it is to get this patch work initially. The only hard thing was "discussing" this with you. I even gave up my resistance to release ffmpeg 3.4 without it, and took care of your considerations (see updated patches I've sent before). Normally you're quick to apply whatever fragile and complicated BS there is, only here you're making an exception. Why the special treatment. > How many future commits will change the way AVFrames move and incorrectly > update all the wraping. developers are not machines this suprising > requirement of (un)wraping on interfaces will not be done correctly > in a few years from now even if you get it all correct now. Again, this is not complicated. Even if it is, it's less complicated than all the tricky things you insist on and defend. > Now add libavformat, libavdevice, libavfilter specific opaque_ref > wraping > > you really consider this to be good design by any definition of "good"? It's probably better than whatever you could come up with. But you didn't even make a suggestion.
On Fri, 13 Oct 2017 19:41:28 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > > On Fri, 6 Oct 2017 00:01:30 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > The opaque_ref wraping is a really bad design. Iam not sure why > > > people defend it. > > > > FFmpeg is full of this design. There are plenty of structs with > > opaque/priv fields that change meaning depending on the context > > (basically how the struct is used or what uses it). It affects all > > decoders, encoders, filters, demuxers, muxers, the av_log() call, > > functions that work with AVClass, AVOption, and probably more. > > what you write is not true > > each decoder, demuxer, ... CLASS has its own type of private context > nothing outside code specific to that class messes with it. > A snow decoder has a snow context. > If the outside structure is moved around its still a snow decoder with > a snow private context. No amount of moving the structure around makes > it invalid. > > OTOH, opaque_ref is defined by the user application. > There is a single user application in the address space. You misunderstand how AVFrame works. AVFrame has an owner, and this owner decides how certain fields are handled. This includes for example the pts fields, whose meaning entirely depend on an undefined timebase. opaque_ref is merely a more advanced case of this. > Before the patch > all AVFrame opaque_ref have the same type, no amount of moving/passing > AVFrames around results in an invalid AVFrame. Wrong. Even a user application could have multiple uses of opaque_ref, all with their own meaning. You can't interpret this field without context about where its value came from. > after the patch this is very different. > Each AVFrame becomes tied to the code that created it, opaque_ref > has a type that outside decoders, is defined by the user application > inside decoders, its a internal structure. > > AVFrames are no longer a universal structure that can simply be passed > around. > This is bad design and it is fragile > > private and opaque are also intended to be very different things. > In FFmpeg private is a internal thing like the internal context of an > encoder. Opaque is a exteral thing, from the user application or caller Why can you access the private fields anyway? This is fragile and bad design. > You may have misunderstood me but > I am against AVFrames depending on the code that created them. Suggest something better. > This is not just about this implementation (which is in fact buggy too), Is it. > Pointing to the implementation issues and bugs was just intended > to show how fragile this is. > I do not understand how one can on one hand see all the problems this > causes yet not see that the API that causes this is what is at fault. > And it is MUCH easier to fix the API than to fix all the issues that > context specific AVFrames would cause Fix what API? Do you even understand for what this is needed? > add a new field to AVFrame if you want a field with semantics that > are different. HAHAHAHAHAHAHAAHAHAHAHA This is a load of badly designed bullshit. This would add another AVFrame field for something that is INTERNAL and IMPLEMENTATION DEFINED to libavcodec. Do you know that we have a field for this, which is supposed to be used by anything that requires INTERNAL and IMPLEMENTATION DEFINED values by the AVFrame users? It's called opaque_ref. > you said you are against this IIRC. But thats the simple, robust and > easy maintainable solution It's not simple, because you STILL can forget to unwrap or wrap the AVFrame properly. Forgetting to unwrap it would be "harmless" in your opinion because the user isn't allowed to read it, while with opaque_ref it would be "dangerous". But the truth is that an "unwrapped" frame doesn't contain the correct data anyway - post processing was not run, and returning the frame in this state to the user is undefined behavior. Skipping unwrapping is a bug. Unwrapping will restore opaque_ref to the value it should be. Please come up with something that is actually better. Anyway, in my opinion, it would be better to disallow opaque_ref being set by the user's get_buffer2 callback. That would solve most of the issues you have with it, but surely you would come up with some crap to block my work anyway. > more so if that field is not void* it could provide type checking > which most people consider a good thing. > > A system simiar to side data for opaque data could be used too, i > would say thats overkill but some people like side data Side data has literally nothing to do with this. Side data types can't be defined by the user anyway. Why do codec/filter/demuxer private fields have no type checking? Or av_log? Or the whole AVOption API? Please answer this. > One entry for the users opaque structure > One entry for the libavcodec private structure > One entry for a future libavfilter private structure One entry for application 1. ... One entry for application 1000000000. GREAT DESIGN > And this way you need no wraping or unwraping, a AVFrame either YOU FUCKING NEED UNWRAPPING FOR POSTPROCESSING Both intended uses, cuvid and videotoolbox, require this. Unwrapping can't be skipped. Skipping unwrapping is a bug. > has some extra opaque fields or it doesnt. nothing could cast them to > the wrong type. Why is such a thing not required for codecs/filters/demuxers? Hell, your beloved mpegvideo pile of garbage could really use this. > This is much more robust than putting all these things in opaque_ref > and spending time and more time and more time writing code wraping > and unwraping at every interface point and then cursing half the > interface and likely in the future fighting over every bit of added > interface as it massivly increases complexity with the wraping All this code is already written. And again, you can NOT SKIP UNWRAPPING. opaque_ref is superior to adding a new AVFrame field for every private use case.
On Fri, Oct 13, 2017 at 09:19:04PM +0200, wm4 wrote: > On Fri, 13 Oct 2017 19:41:28 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > > > On Fri, 6 Oct 2017 00:01:30 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > The opaque_ref wraping is a really bad design. Iam not sure why > > > > people defend it. > > > > > > FFmpeg is full of this design. There are plenty of structs with > > > opaque/priv fields that change meaning depending on the context > > > (basically how the struct is used or what uses it). It affects all > > > decoders, encoders, filters, demuxers, muxers, the av_log() call, > > > functions that work with AVClass, AVOption, and probably more. > > > > what you write is not true > > > > each decoder, demuxer, ... CLASS has its own type of private context > > nothing outside code specific to that class messes with it. > > A snow decoder has a snow context. > > If the outside structure is moved around its still a snow decoder with > > a snow private context. No amount of moving the structure around makes > > it invalid. > > > > OTOH, opaque_ref is defined by the user application. > > There is a single user application in the address space. > > You misunderstand how AVFrame works. AVFrame has an owner, and this > owner decides how certain fields are handled. This includes for example > the pts fields, whose meaning entirely depend on an undefined timebase. > opaque_ref is merely a more advanced case of this. The API docs say about opaque_ref "FFmpeg will never check the contents of the buffer ref." the unwraping code does exactly that and any code using it internally does as well. Let me quote the code from one of these patches: diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 437b848248..04f7156154 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c [...] + if (frame->opaque_ref) { + FrameDecodeData *fdd; + AVBufferRef *user_opaque_ref; + + fdd = (FrameDecodeData*)frame->opaque_ref->data; This violates the API and it is semantically wrong. about pts, that is off topic, but no amount of moving an AVFrame around makes a valid AVFrame pts into an invalid AVFrame pts. AVFrame pts refers to a unspecified timebase either way. If the code knows that timebase and knows the origin it can interpret the timestamp. It may make sense to add more information about the pts into AVFrame so its more self contained, but this is off topic > > > Before the patch > > all AVFrame opaque_ref have the same type, no amount of moving/passing > > AVFrames around results in an invalid AVFrame. > > Wrong. Even a user application could have multiple uses of opaque_ref, > all with their own meaning. You can't interpret this field without > context about where its value came from. No matter if the user app uses 1, 2 or n types in opaque_ref. Its still at all points the user apps type and thus the same type for this discussions point of view. interpreting the user apps opaque_ref is entirely in the user applications domain. We can never interpret it. The user application should know how to interpret its own data. [...] > > more so if that field is not void* it could provide type checking > > which most people consider a good thing. > > > > A system simiar to side data for opaque data could be used too, i > > would say thats overkill but some people like side data > > Side data has literally nothing to do with this. Side data types can't > be defined by the user anyway. > Why do codec/filter/demuxer private > fields have no type checking? Or av_log? Or the whole AVOption API? > Please answer this. This thread is the wrong place to discuss this, this is off topic Of course it would be good if we can check types in more cases ... [...] > > And this way you need no wraping or unwraping, a AVFrame either > > YOU FUCKING NEED UNWRAPPING FOR POSTPROCESSING > > Both intended uses, cuvid and videotoolbox, require this. Unwrapping > can't be skipped. Skipping unwrapping is a bug. IIUC these need a postprocessing function to be called. There is no relation between calling a postprocessing function and wraping and unwraping the users opaque_ref. You can do either without the other. [...]
On Sat, 14 Oct 2017 23:01:41 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Oct 13, 2017 at 09:19:04PM +0200, wm4 wrote: > > On Fri, 13 Oct 2017 19:41:28 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > > > > On Fri, 6 Oct 2017 00:01:30 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > The opaque_ref wraping is a really bad design. Iam not sure why > > > > > people defend it. > > > > > > > > FFmpeg is full of this design. There are plenty of structs with > > > > opaque/priv fields that change meaning depending on the context > > > > (basically how the struct is used or what uses it). It affects all > > > > decoders, encoders, filters, demuxers, muxers, the av_log() call, > > > > functions that work with AVClass, AVOption, and probably more. > > > > > > what you write is not true > > > > > > each decoder, demuxer, ... CLASS has its own type of private context > > > nothing outside code specific to that class messes with it. > > > A snow decoder has a snow context. > > > If the outside structure is moved around its still a snow decoder with > > > a snow private context. No amount of moving the structure around makes > > > it invalid. > > > > > > OTOH, opaque_ref is defined by the user application. > > > There is a single user application in the address space. > > > > You misunderstand how AVFrame works. AVFrame has an owner, and this > > owner decides how certain fields are handled. This includes for example > > the pts fields, whose meaning entirely depend on an undefined timebase. > > opaque_ref is merely a more advanced case of this. > > The API docs say about opaque_ref > "FFmpeg will never check the contents of the buffer ref." > the unwraping code does exactly that and any code using it internally > does as well. It doesn't - not the user's. We use only the field for internal purposes (as AVFrame users), and we never do anything with the user's value. I'm not sure if you get this, so let me repeat this. We never interpret the user's value, nor do we return our own internal value of this to the user. Make sure you understand this. > Let me quote the code from one of these patches: > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 437b848248..04f7156154 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > [...] > + if (frame->opaque_ref) { > + FrameDecodeData *fdd; > + AVBufferRef *user_opaque_ref; > + > + fdd = (FrameDecodeData*)frame->opaque_ref->data; > > This violates the API and it is semantically wrong. No it does not. Cease your bullshit. It never inspects the user value, it only wraps it. Stop trying to misrepresent it or inventing semantics on top of the doxygen (which I write btw.). > > Wrong. Even a user application could have multiple uses of opaque_ref, > > all with their own meaning. You can't interpret this field without > > context about where its value came from. > > No matter if the user app uses 1, 2 or n types in opaque_ref. > Its still at all points the user apps type and thus the same type for > this discussions point of view. No. The user app could have multiple uses of it with multiple types, at different points in the program. > interpreting the user apps opaque_ref is entirely in the user > applications domain. We can never interpret it. The user application > should know how to interpret its own data. And we never interpret it. Why do you think we do? Am I talking to a fucking wall? A bot? > > Both intended uses, cuvid and videotoolbox, require this. Unwrapping > > can't be skipped. Skipping unwrapping is a bug. > > IIUC these need a postprocessing function to be called. > There is no relation between calling a postprocessing function and > wraping and unwraping the users opaque_ref. You can do either without > the other. Yes there is. You claim opaque_ref is "unsafe" because you could forget unwrapping it. But forgetting unwrapping is equivalent to forgetting calling the postprocessing, which would be a bug. Why is one kind of bug so critical that leads you to reject this patch, while the you barely acknowledge the other bug and don't care? Unless you have real arguments, I'll push the updated patches in 24h.
On 10/03/2017 09:15 PM, wm4 wrote: > From: Anton Khirnov <anton@khirnov.net> > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > wrapped in the lavc struct and then unwrapped before the frame is > returned to the caller. > > This new struct will be useful in the following commits. > How about using AVFrame::buf[]?
On Mon, 16 Oct 2017 19:28:27 +0800 Xiaolei Yu <dreifachstein@gmail.com> wrote: > On 10/03/2017 09:15 PM, wm4 wrote: > > From: Anton Khirnov <anton@khirnov.net> > > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is > > wrapped in the lavc struct and then unwrapped before the frame is > > returned to the caller. > > > > This new struct will be useful in the following commits. > > > > How about using AVFrame::buf[]? My local crash-fix hack for videotoolbox actually uses buf[] for this purpose, but it won't work in general, because video data might need it. Also that would have exactly the same problems as opaque_ref. Actually, all approaches will have exactly the same problems as opaque_ref, but michael doesn't get it.
On 10/16/2017 07:36 PM, wm4 wrote: > On Mon, 16 Oct 2017 19:28:27 +0800 > Xiaolei Yu <dreifachstein@gmail.com> wrote: > >> On 10/03/2017 09:15 PM, wm4 wrote: >>> From: Anton Khirnov <anton@khirnov.net> >>> >>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is >>> wrapped in the lavc struct and then unwrapped before the frame is >>> returned to the caller. >>> >>> This new struct will be useful in the following commits. >>> >> >> How about using AVFrame::buf[]? > > My local crash-fix hack for videotoolbox actually uses buf[] for this > purpose, but it won't work in general, because video data might need > it. Also that would have exactly the same problems as opaque_ref. > Actually, all approaches will have exactly the same problems as > opaque_ref, but michael doesn't get it. I think handling foreign memory with a cpu-centric api will always be kinda awkward. But using AVFrame::buf[] is more natural if we treat them as a family of cpu-unaddressable images. Also it would be great (in the future) to allow users to pass them around without a round trip to the main memory. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, 16 Oct 2017 20:32:03 +0800 Xiaolei Yu <dreifachstein@gmail.com> wrote: > On 10/16/2017 07:36 PM, wm4 wrote: > > On Mon, 16 Oct 2017 19:28:27 +0800 > > Xiaolei Yu <dreifachstein@gmail.com> wrote: > > > >> On 10/03/2017 09:15 PM, wm4 wrote: > >>> From: Anton Khirnov <anton@khirnov.net> > >>> > >>> Use the AVFrame.opaque_ref field. The original user's opaque_ref is > >>> wrapped in the lavc struct and then unwrapped before the frame is > >>> returned to the caller. > >>> > >>> This new struct will be useful in the following commits. > >>> > >> > >> How about using AVFrame::buf[]? > > > > My local crash-fix hack for videotoolbox actually uses buf[] for this > > purpose, but it won't work in general, because video data might need > > it. Also that would have exactly the same problems as opaque_ref. > > Actually, all approaches will have exactly the same problems as > > opaque_ref, but michael doesn't get it. > > I think handling foreign memory with a cpu-centric api will always be kinda > awkward. But using AVFrame::buf[] is more natural if we treat them as a > family of cpu-unaddressable images. Also it would be great (in the future) > to allow users to pass them around without a round trip to the main memory. No, AVFrame.buf[] always points to CPU addressable memory. For hardware decoding, the AVFrame.buf[0] usually points to a dummy AVBufferRef (with either size 0, or pointing towards some other struct describing the hardware surface). Not sure how this relates to the current discussion. The problem with videotoolbox is that AVHWAccel.end_frame can be run on a worker thread, and videotoolbox needs to set the actual hardware surface in that callback (because Apple's API returns the frame once all decoding is done, and you can't pass a user-allocated surface to it). The worker thread can't propagate its AVFrame changes back to the main thread (where another AVFrame reference to the same frame is returned). Thus a post process callback is required, and the AVFrame within libavcodec is always invalid from a user point of view until post process is invoked.
On Mon, Oct 16, 2017 at 09:40:26AM +0200, wm4 wrote: > On Sat, 14 Oct 2017 23:01:41 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, Oct 13, 2017 at 09:19:04PM +0200, wm4 wrote: > > > On Fri, 13 Oct 2017 19:41:28 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > > > > > On Fri, 6 Oct 2017 00:01:30 +0200 > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > The opaque_ref wraping is a really bad design. Iam not sure why > > > > > > people defend it. > > > > > > > > > > FFmpeg is full of this design. There are plenty of structs with > > > > > opaque/priv fields that change meaning depending on the context > > > > > (basically how the struct is used or what uses it). It affects all > > > > > decoders, encoders, filters, demuxers, muxers, the av_log() call, > > > > > functions that work with AVClass, AVOption, and probably more. > > > > > > > > what you write is not true > > > > > > > > each decoder, demuxer, ... CLASS has its own type of private context > > > > nothing outside code specific to that class messes with it. > > > > A snow decoder has a snow context. > > > > If the outside structure is moved around its still a snow decoder with > > > > a snow private context. No amount of moving the structure around makes > > > > it invalid. > > > > > > > > OTOH, opaque_ref is defined by the user application. > > > > There is a single user application in the address space. > > > > > > You misunderstand how AVFrame works. AVFrame has an owner, and this > > > owner decides how certain fields are handled. This includes for example > > > the pts fields, whose meaning entirely depend on an undefined timebase. > > > opaque_ref is merely a more advanced case of this. > > > > The API docs say about opaque_ref > > "FFmpeg will never check the contents of the buffer ref." > > the unwraping code does exactly that and any code using it internally > > does as well. > > It doesn't - not the user's. We use only the field for internal > purposes (as AVFrame users), and we never do anything with the user's > value. > > I'm not sure if you get this, so let me repeat this. We never interpret > the user's value, nor do we return our own internal value of this to > the user. Make sure you understand this. sure, the patch uses the opaque_ref field for its libavcodec internal purposes Theres where the need for wraping and unwraping comes from. These are 2 semantically distinct use cases that do not fit well in a single field. One can put anything in any (large enough) field of any structure that way. On all input APIs replace the field by a new structure and put the original fields content in that structure. On output take the field of the internal struct, free the structure and write the original fields value back Thats exactly what this patches wraping code does. And thats what I called a fragile hack. it also violates the API IMO, but thats not so much the point The data the user application wants to attach to a AVFrame for the user applications extrenal purposes and The data libavcodec wants to attach for internal (hwaccel) use. Are 2 distinct things. You can have none, either one or both theres no relation between them. [...] > > > > Both intended uses, cuvid and videotoolbox, require this. Unwrapping > > > can't be skipped. Skipping unwrapping is a bug. > > > > IIUC these need a postprocessing function to be called. > > There is no relation between calling a postprocessing function and > > wraping and unwraping the users opaque_ref. You can do either without > > the other. > > Yes there is. You claim opaque_ref is "unsafe" because you could forget > unwrapping it. But forgetting unwrapping is equivalent to forgetting > calling the postprocessing, which would be a bug. We can discuss about cases where unwraping is needed while postprocessing is not but i think this would distract from the subject so it would be better in a different thread. > > Why is one kind of bug so critical that leads you to reject this patch, > while the you barely acknowledge the other bug and don't care? > > Unless you have real arguments, I'll push the updated patches in 24h. If you wish to push this patch against my veto, you have to get the vote comittee to override my veto. There is no need for you to accept my arguments. Iam interrested to hear the argumentation of other developers why this code is not a hack, not fragile, not hard to maintain, not a security risk and why we should go this path instead of some alternative without these issues. Maybe iam totally wrong and everyone feels this patch is the way code should be written and API designed. But it would surprise me alot if there are alot of people supporting this kind of design Thanks [...]
I have realized that your veto is actually not valid: - it's a Libav merge - it has been for months in the Libav repo and you didn't specifically care, nor did you make an attempt to merge the commit in a "fixed" way - this patch would have been merged normally, and you wouldn't have cared at all about it So unless you intend to make a better, working proposal, I will _not_ allow you to make this "my" problem. I will psuh this in 3 hours. After that, you're free to to reimplement this in a different way or whatever as a merge cleanup. On Tue, 17 Oct 2017 00:58:58 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Oct 16, 2017 at 09:40:26AM +0200, wm4 wrote: > > On Sat, 14 Oct 2017 23:01:41 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Fri, Oct 13, 2017 at 09:19:04PM +0200, wm4 wrote: > > > > On Fri, 13 Oct 2017 19:41:28 +0200 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote: > > > > > > On Fri, 6 Oct 2017 00:01:30 +0200 > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > The opaque_ref wraping is a really bad design. Iam not sure why > > > > > > > people defend it. > > > > > > > > > > > > FFmpeg is full of this design. There are plenty of structs with > > > > > > opaque/priv fields that change meaning depending on the context > > > > > > (basically how the struct is used or what uses it). It affects all > > > > > > decoders, encoders, filters, demuxers, muxers, the av_log() call, > > > > > > functions that work with AVClass, AVOption, and probably more. > > > > > > > > > > what you write is not true > > > > > > > > > > each decoder, demuxer, ... CLASS has its own type of private context > > > > > nothing outside code specific to that class messes with it. > > > > > A snow decoder has a snow context. > > > > > If the outside structure is moved around its still a snow decoder with > > > > > a snow private context. No amount of moving the structure around makes > > > > > it invalid. > > > > > > > > > > OTOH, opaque_ref is defined by the user application. > > > > > There is a single user application in the address space. > > > > > > > > You misunderstand how AVFrame works. AVFrame has an owner, and this > > > > owner decides how certain fields are handled. This includes for example > > > > the pts fields, whose meaning entirely depend on an undefined timebase. > > > > opaque_ref is merely a more advanced case of this. > > > > > > The API docs say about opaque_ref > > > "FFmpeg will never check the contents of the buffer ref." > > > the unwraping code does exactly that and any code using it internally > > > does as well. > > > > > It doesn't - not the user's. We use only the field for internal > > purposes (as AVFrame users), and we never do anything with the user's > > value. > > > > I'm not sure if you get this, so let me repeat this. We never interpret > > the user's value, nor do we return our own internal value of this to > > the user. Make sure you understand this. > > sure, the patch uses the opaque_ref field for its libavcodec internal > purposes > > Theres where the need for wraping and unwraping comes from. > These are 2 semantically distinct use cases that do not fit well in a > single field. > > One can put anything in any (large enough) field of any structure that > way. > On all input APIs replace the field by a new structure and put > the original fields content in that structure. > On output take the field of the internal struct, free the structure > and write the original fields value back > Thats exactly what this patches wraping code does. > > And thats what I called a fragile hack. > > it also violates the API IMO, but thats not so much the point > > The data the user application wants to attach to a AVFrame for the > user applications extrenal purposes > and > The data libavcodec wants to attach for internal (hwaccel) use. > > Are 2 distinct things. You can have none, either one or both theres > no relation between them. > > > [...] > > > > > > Both intended uses, cuvid and videotoolbox, require this. Unwrapping > > > > can't be skipped. Skipping unwrapping is a bug. > > > > > > IIUC these need a postprocessing function to be called. > > > There is no relation between calling a postprocessing function and > > > wraping and unwraping the users opaque_ref. You can do either without > > > the other. > > > > > Yes there is. You claim opaque_ref is "unsafe" because you could forget > > unwrapping it. But forgetting unwrapping is equivalent to forgetting > > calling the postprocessing, which would be a bug. > > We can discuss about cases where unwraping is needed while postprocessing > is not but i think this would distract from the subject so it would be > better in a different thread. > > > > > > Why is one kind of bug so critical that leads you to reject this patch, > > while the you barely acknowledge the other bug and don't care? > > > > > Unless you have real arguments, I'll push the updated patches in 24h. > > If you wish to push this patch against my veto, you have to get the > vote comittee to override my veto. There is no need for you to accept > my arguments. > > Iam interrested to hear the argumentation of other developers > why this code is not a hack, not fragile, not hard to maintain, not a > security risk and why we should go this path instead of some > alternative without these issues. > > Maybe iam totally wrong and everyone feels this patch is the way code > should be written and API designed. > But it would surprise me alot if there are alot of people supporting > this kind of design > > Thanks > > [...]
Correction: will push as part of cuvid/videotoolbox patches whenever they're ready. On Tue, 17 Oct 2017 00:58:58 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > > It doesn't - not the user's. We use only the field for internal > > purposes (as AVFrame users), and we never do anything with the user's > > value. > > > > I'm not sure if you get this, so let me repeat this. We never interpret > > the user's value, nor do we return our own internal value of this to > > the user. Make sure you understand this. > > sure, the patch uses the opaque_ref field for its libavcodec internal > purposes Which it is allowed to. > Theres where the need for wraping and unwraping comes from. > These are 2 semantically distinct use cases that do not fit well in a > single field. There's nothing semantically distinct. The opaque_ref field's semantics are defined by the owner of the AVFrame, and when ownership is passed through the components, rewrapping might be needed. Which is what the patch does. How often did I repeat this and how often did you not understand this? > One can put anything in any (large enough) field of any structure that > way. > On all input APIs replace the field by a new structure and put > the original fields content in that structure. > On output take the field of the internal struct, free the structure > and write the original fields value back > Thats exactly what this patches wraping code does. > > And thats what I called a fragile hack. Anything you suggested is even worse and even more fragile. > it also violates the API IMO, but thats not so much the point It does not. I created the fucking API. > The data the user application wants to attach to a AVFrame for the > user applications extrenal purposes > and > The data libavcodec wants to attach for internal (hwaccel) use. > > Are 2 distinct things. You can have none, either one or both theres > no relation between them. AVFrame does not dictate a use for this field. The use of the field is determined by the owner of the AVFrame, which is libavcodec while the frame is in libavcodec. How often did I repeat this and how often did you not understand this? > > [...] > > > > > > Both intended uses, cuvid and videotoolbox, require this. Unwrapping > > > > can't be skipped. Skipping unwrapping is a bug. > > > > > > IIUC these need a postprocessing function to be called. > > > There is no relation between calling a postprocessing function and > > > wraping and unwraping the users opaque_ref. You can do either without > > > the other. > > > > > Yes there is. You claim opaque_ref is "unsafe" because you could forget > > unwrapping it. But forgetting unwrapping is equivalent to forgetting > > calling the postprocessing, which would be a bug. > > We can discuss about cases where unwraping is needed while postprocessing > is not but i think this would distract from the subject so it would be > better in a different thread. The fuck? Unwrapping is equivalent to doing postprocessing required to make it a valid output frame. > > > > > Why is one kind of bug so critical that leads you to reject this patch, > > while the you barely acknowledge the other bug and don't care? > > > > > Unless you have real arguments, I'll push the updated patches in 24h. > > If you wish to push this patch against my veto, you have to get the > vote comittee to override my veto. There is no need for you to accept > my arguments. There's no need for me to justify merging a Libav patch, although I've wasted more than enough time doing so. If you don't like Libav patches, maybe you should never have forked. > Iam interrested to hear the argumentation of other developers > why this code is not a hack, not fragile, not hard to maintain, not a > security risk and why we should go this path instead of some > alternative without these issues. > > Maybe iam totally wrong and everyone feels this patch is the way code > should be written and API designed. > But it would surprise me alot if there are alot of people supporting > this kind of design > > Thanks > > [...]
Am 17.10.17 um 06:49 schrieb wm4: > I have realized that your veto is actually not valid: > - it's a Libav merge > - it has been for months in the Libav repo and you didn't specifically > care, nor did you make an attempt to merge the commit in a "fixed" way > - this patch would have been merged normally, and you wouldn't have > cared at all about it > > So unless you intend to make a better, working proposal, I will _not_ > allow you to make this "my" problem. I will psuh this in 3 hours. After > that, you're free to to reimplement this in a different way or whatever > as a merge cleanup. Can you please stop this and just find a reasonable way to find consensus on the issues you're facing? Is it really of such importance that you want to push this although there are still concerns about what you want to do? Is this at all in any way relevant for security so that time would be at all a factor? I don't see any of your arguments mentioned above to be a valid reason to overrule someone else's opinion. And no, I do not have the slightest clue about what you folks are discussing about but this should not even matter - I just criticize the way you are going to take here... -Thilo
On Tue, 17 Oct 2017 20:23:25 +0200 Thilo Borgmann <thilo.borgmann@mail.de> wrote: > Am 17.10.17 um 06:49 schrieb wm4: > > I have realized that your veto is actually not valid: > > - it's a Libav merge > > - it has been for months in the Libav repo and you didn't specifically > > care, nor did you make an attempt to merge the commit in a "fixed" way > > - this patch would have been merged normally, and you wouldn't have > > cared at all about it > > > > So unless you intend to make a better, working proposal, I will _not_ > > allow you to make this "my" problem. I will psuh this in 3 hours. After > > that, you're free to to reimplement this in a different way or whatever > > as a merge cleanup. > > Can you please stop this and just find a reasonable way to find consensus on the issues you're facing? > > Is it really of such importance that you want to push this although there are still concerns about what you want to do? Is this at all in any way relevant for security so that time would be at all a factor? I don't see any of your arguments mentioned above to be a valid reason to overrule someone else's opinion. I don't want trivial things getting blocked by pure bullshit that's probably politically and not technically motivated. The best part about this dumb shit is that I could probably send patches for cuvid/videotoolbox that use dumb workarounds (i.e. uglier and shittier hacks) than the discussed patch, and NOBODY WOULD FUCKING CARE. I'm pretty fed up with this shit. Please keep out of this discussion if you can't contribute anything too.
Am 17.10.17 um 20:27 schrieb wm4: > On Tue, 17 Oct 2017 20:23:25 +0200 > Thilo Borgmann <thilo.borgmann@mail.de> wrote: > >> Am 17.10.17 um 06:49 schrieb wm4: >>> I have realized that your veto is actually not valid: >>> - it's a Libav merge >>> - it has been for months in the Libav repo and you didn't specifically >>> care, nor did you make an attempt to merge the commit in a "fixed" way >>> - this patch would have been merged normally, and you wouldn't have >>> cared at all about it >>> >>> So unless you intend to make a better, working proposal, I will _not_ >>> allow you to make this "my" problem. I will psuh this in 3 hours. After >>> that, you're free to to reimplement this in a different way or whatever >>> as a merge cleanup. >> >> Can you please stop this and just find a reasonable way to find consensus on the issues you're facing? >> >> Is it really of such importance that you want to push this although there are still concerns about what you want to do? Is this at all in any way relevant for security so that time would be at all a factor? I don't see any of your arguments mentioned above to be a valid reason to overrule someone else's opinion. > > I don't want trivial things getting blocked by pure bullshit that's > probably politically and not technically motivated. > > The best part about this dumb shit is that I could probably send > patches for cuvid/videotoolbox that use dumb workarounds (i.e. uglier > and shittier hacks) than the discussed patch, and NOBODY WOULD FUCKING > CARE. > > I'm pretty fed up with this shit. > > Please keep out of this discussion if you can't contribute anything too. I do care because I am delaying my work on some other cuvid related thing because of this. I would prefer not to dive into this topic any further because it seems rather "not so important" to my task. And just another cook in the kitchen would also more likely avoid conensus here. All I ask for is for you to find a reasonable to argue about your issues. I hope you see my point why I am raising my voice. Thanks! -Thilo
Am 17.10.17 um 20:37 schrieb Thilo Borgmann: > Am 17.10.17 um 20:27 schrieb wm4: >> On Tue, 17 Oct 2017 20:23:25 +0200 >> Thilo Borgmann <thilo.borgmann@mail.de> wrote: >> >>> Am 17.10.17 um 06:49 schrieb wm4: >>>> I have realized that your veto is actually not valid: >>>> - it's a Libav merge >>>> - it has been for months in the Libav repo and you didn't specifically >>>> care, nor did you make an attempt to merge the commit in a "fixed" way >>>> - this patch would have been merged normally, and you wouldn't have >>>> cared at all about it >>>> >>>> So unless you intend to make a better, working proposal, I will _not_ >>>> allow you to make this "my" problem. I will psuh this in 3 hours. After >>>> that, you're free to to reimplement this in a different way or whatever >>>> as a merge cleanup. >>> >>> Can you please stop this and just find a reasonable way to find consensus on the issues you're facing? >>> >>> Is it really of such importance that you want to push this although there are still concerns about what you want to do? Is this at all in any way relevant for security so that time would be at all a factor? I don't see any of your arguments mentioned above to be a valid reason to overrule someone else's opinion. >> >> I don't want trivial things getting blocked by pure bullshit that's >> probably politically and not technically motivated. >> >> The best part about this dumb shit is that I could probably send >> patches for cuvid/videotoolbox that use dumb workarounds (i.e. uglier >> and shittier hacks) than the discussed patch, and NOBODY WOULD FUCKING >> CARE. >> >> I'm pretty fed up with this shit. >> >> Please keep out of this discussion if you can't contribute anything too. > > I do care because I am delaying my work on some other cuvid related thing because of this. > > I would prefer not to dive into this topic any further because it seems rather "not so important" to my task. And just another cook in the kitchen would also more likely avoid conensus here. > > All I ask for is for you to find a reasonable to argue about your issues. ^ way > I hope you see my point why I am raising my voice. Thanks! > > -Thilo
Am 17.10.2017 um 20:37 schrieb Thilo Borgmann: > Am 17.10.17 um 20:27 schrieb wm4: >> On Tue, 17 Oct 2017 20:23:25 +0200 >> Thilo Borgmann <thilo.borgmann@mail.de> wrote: >> >>> Am 17.10.17 um 06:49 schrieb wm4: >>>> I have realized that your veto is actually not valid: >>>> - it's a Libav merge >>>> - it has been for months in the Libav repo and you didn't specifically >>>> care, nor did you make an attempt to merge the commit in a "fixed" way >>>> - this patch would have been merged normally, and you wouldn't have >>>> cared at all about it >>>> >>>> So unless you intend to make a better, working proposal, I will _not_ >>>> allow you to make this "my" problem. I will psuh this in 3 hours. After >>>> that, you're free to to reimplement this in a different way or whatever >>>> as a merge cleanup. >>> >>> Can you please stop this and just find a reasonable way to find consensus on the issues you're facing? >>> >>> Is it really of such importance that you want to push this although there are still concerns about what you want to do? Is this at all in any way relevant for security so that time would be at all a factor? I don't see any of your arguments mentioned above to be a valid reason to overrule someone else's opinion. >> >> I don't want trivial things getting blocked by pure bullshit that's >> probably politically and not technically motivated. >> >> The best part about this dumb shit is that I could probably send >> patches for cuvid/videotoolbox that use dumb workarounds (i.e. uglier >> and shittier hacks) than the discussed patch, and NOBODY WOULD FUCKING >> CARE. >> >> I'm pretty fed up with this shit. >> >> Please keep out of this discussion if you can't contribute anything too. > > I do care because I am delaying my work on some other cuvid related thing because of this. > > I would prefer not to dive into this topic any further because it seems rather "not so important" to my task. And just another cook in the kitchen would also more likely avoid conensus here. > > All I ask for is for you to find a reasonable to argue about your issues. > > I hope you see my point why I am raising my voice. Thanks! > > -Thilo I second this. The exact way this is implemented is entirely unimportant. But this pointless fighting leads nowhere. While I agree that this patch is technically correct and does not violate API in any way, I also kind of dislike using opaque_ref for this, and would welcome coming up with a nicer solution, that's ideally compatible with libavs approach without too much work on every patch that touches it.
On Tue, Oct 17, 2017 at 06:49:51AM +0200, wm4 wrote: > I have realized that your veto is actually not valid: > - it's a Libav merge There is no exception for changes from Libav. Not in the policy and not in actual actions. We never pushed changes with standing objections. [...]
On Tue, Oct 17, 2017 at 07:26:01AM +0200, wm4 wrote: [...] > > > it also violates the API IMO, but thats not so much the point > > It does not. I created the fucking API. > > > The data the user application wants to attach to a AVFrame for the > > user applications extrenal purposes > > and > > The data libavcodec wants to attach for internal (hwaccel) use. > > > > Are 2 distinct things. You can have none, either one or both theres > > no relation between them. > > AVFrame does not dictate a use for this field. The use of the field is > determined by the owner of the AVFrame, which is libavcodec while the > frame is in libavcodec. > > How often did I repeat this and how often did you not understand this? opaque_ref is defined to have the same semantics as opaque. opaque definitly does NOT have the semantics you write about in this thread. opaque is a field that exists for a very long time I think you and Libav misunderstood the opaque field semantics Just think about it, the opaque field existed when the *frame structure was defined in libavcodec. The user of libavcodec is NOT libavcodec This field was never intended to be used by any of our libs to store their private data. It was the move of AVFrame to libavutil that made the documentation of opaque ambigous and this likely lead to this misunderstanding
On Tue, 17 Oct 2017 20:37:32 +0200, Thilo Borgmann <thilo.borgmann@mail.de> wrote: > I do care because I am delaying my work on some other cuvid related thing because of this. > > I would prefer not to dive into this topic any further because it seems rather "not so important" to my task. And just another cook in the kitchen would also more likely avoid conensus here. thilo, can we bribe you to massage this patch a little? it seems like wm4 and michael both are not budging... a third party might be able to settle the stalemate. -compn
On Wed, 4 Oct 2017 11:34:18 +0200, wm4 <nfxjfg@googlemail.com> wrote: > > more so, opaque_ref is used in only 5 lines in the whole codebase, > > so there is not much code to consider when using a different solution > > We shouldn't add such special fields, we have enough hacks already. Is > that your only suggestion how to do this? Because it's a bad one. michael, do you have other suggestions how to solve this problem? are you going to fix the errors in the doxygen that you noticed due to merges? wm4, would you object to adding another special field? if someone else edited this and made a patch? -compn
On Wed, Oct 18, 2017 at 09:48:16AM -0400, Compn wrote:
[...]
> a third party might be able to settle the stalemate.
i strongly support this. And it was in part why i suggested to take
this to the vote comittee. Most people from the vote committee are
uninvolved in this
This should be carefully looked at by people with cool heads and
decided.
I had actually written something similar in my last reply but i edited
it out before sending as it seemed to get a bit lengthy and redundant.
[...]
On Wed, 18 Oct 2017, Michael Niedermayer wrote: > On Wed, Oct 18, 2017 at 09:48:16AM -0400, Compn wrote: > [...] > >> a third party might be able to settle the stalemate. > > i strongly support this. And it was in part why i suggested to take > this to the vote comittee. Most people from the vote committee are > uninvolved in this > > This should be carefully looked at by people with cool heads and > decided. I tried to follow the discussion but I still don't get why adding a new "priv_ref" field to AVFrame for lavc internal use is frowned upon. It seems like the easiest solution to relax the concerns and a small price to pay to put this story behind us. Unfortunately I don't know much about hwaccel, so I can't judge the whole patch series, but for me it is not evident that post-processing belongs to the decoder interface, if everybody agrees that it does, then OK. I also find storing an AVBufferRef in an AVBufferRef a bit confusing, because sooner or later we won't know if we have to do a deep ref on referencing an avframe, or not, so I'd try to avoid that in general. Regards, Marton
Am 18.10.17 um 16:02 schrieb Compn: > On Wed, 4 Oct 2017 11:34:18 +0200, wm4 <nfxjfg@googlemail.com> wrote: > >>> more so, opaque_ref is used in only 5 lines in the whole codebase, >>> so there is not much code to consider when using a different solution >> >> We shouldn't add such special fields, we have enough hacks already. Is >> that your only suggestion how to do this? Because it's a bad one. > > michael, do you have other suggestions how to solve this problem? are > you going to fix the errors in the doxygen that you noticed due to > merges? > > wm4, would you object to adding another special field? if > someone else edited this and made a patch? If the two of you could agree on an alternate, suitable approach, I could offer to do the codemonkey's work like compn suggested. However, I still lack some knowledge about API and HWACCEL which would result in certain inefficiency in doing so. OTOH this comes along with a nonbiased perspective which might be a positive thing here. I'd appreciate it if anyone else would be willing to commit some coding to a solution, though. I am not eager to do it but I can if we stall completely otherwise. -Thilo
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 437b848248..04f7156154 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -640,6 +640,26 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) if (ret == AVERROR_EOF) avci->draining_done = 1; + /* unwrap the per-frame decode data and restore the original opaque_ref*/ + if (!ret) { + /* the only case where decode data is not set should be decoders + * that do not call ff_get_buffer() */ + av_assert0((frame->opaque_ref && frame->opaque_ref->size == sizeof(FrameDecodeData)) || + !(avctx->codec->capabilities & AV_CODEC_CAP_DR1)); + + if (frame->opaque_ref) { + FrameDecodeData *fdd; + AVBufferRef *user_opaque_ref; + + fdd = (FrameDecodeData*)frame->opaque_ref->data; + + user_opaque_ref = fdd->user_opaque_ref; + fdd->user_opaque_ref = NULL; + av_buffer_unref(&frame->opaque_ref); + frame->opaque_ref = user_opaque_ref; + } + } + return ret; } @@ -1612,6 +1632,37 @@ static void validate_avframe_allocation(AVCodecContext *avctx, AVFrame *frame) } } +static void decode_data_free(void *opaque, uint8_t *data) +{ + FrameDecodeData *fdd = (FrameDecodeData*)data; + + av_buffer_unref(&fdd->user_opaque_ref); + + av_freep(&fdd); +} + +static int attach_decode_data(AVFrame *frame) +{ + AVBufferRef *fdd_buf; + FrameDecodeData *fdd; + + fdd = av_mallocz(sizeof(*fdd)); + if (!fdd) + return AVERROR(ENOMEM); + + fdd_buf = av_buffer_create((uint8_t*)fdd, sizeof(*fdd), decode_data_free, + NULL, AV_BUFFER_FLAG_READONLY); + if (!fdd_buf) { + av_freep(&fdd); + return AVERROR(ENOMEM); + } + + fdd->user_opaque_ref = frame->opaque_ref; + frame->opaque_ref = fdd_buf; + + return 0; +} + static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int flags) { const AVHWAccel *hwaccel = avctx->hwaccel; @@ -1648,8 +1699,14 @@ static int get_buffer_internal(AVCodecContext *avctx, AVFrame *frame, int flags) avctx->sw_pix_fmt = avctx->pix_fmt; ret = avctx->get_buffer2(avctx, frame, flags); - if (ret >= 0) - validate_avframe_allocation(avctx, frame); + if (ret < 0) + goto end; + + validate_avframe_allocation(avctx, frame); + + ret = attach_decode_data(frame); + if (ret < 0) + goto end; end: if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && !override_dimensions && diff --git a/libavcodec/decode.h b/libavcodec/decode.h index c9630228dc..ab8327f95c 100644 --- a/libavcodec/decode.h +++ b/libavcodec/decode.h @@ -21,8 +21,21 @@ #ifndef AVCODEC_DECODE_H #define AVCODEC_DECODE_H +#include "libavutil/buffer.h" + #include "avcodec.h" +/** + * This struct stores per-frame lavc-internal data and is attached to it via + * opaque_ref. + */ +typedef struct FrameDecodeData { + /** + * The original user-set opaque_ref. + */ + AVBufferRef *user_opaque_ref; +} FrameDecodeData; + /** * Called by decoders to get the next packet for decoding. *
From: Anton Khirnov <anton@khirnov.net> Use the AVFrame.opaque_ref field. The original user's opaque_ref is wrapped in the lavc struct and then unwrapped before the frame is returned to the caller. This new struct will be useful in the following commits. Merges Libav commit 359a8a3e2d1194b52b6c386f94fd0929567dfb67. --- libavcodec/decode.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-- libavcodec/decode.h | 13 ++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-)