On Mon, Oct 16, 2017 at 09:40:26AM +0200, wm4 wrote: > On Sat, 14 Oct 2017 23:01:41 +0200 > Michael Niedermayer <mich...@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 <mich...@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 <mich...@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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
