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 <mich...@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 <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 > > [...] _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
