On Sat, Oct 23, 2021 at 12:13 AM Jan Ekström <jee...@gmail.com> wrote: > > On Sat, Oct 23, 2021 at 12:11 AM Jan Ekström <jee...@gmail.com> wrote: > > > > On Fri, Oct 22, 2021 at 11:26 PM Michael Niedermayer > > <mich...@niedermayer.cc> wrote: > > > > > > On Fri, Oct 22, 2021 at 08:42:14PM +0300, Jan Ekström wrote: > > > > On Fri, Oct 22, 2021 at 6:16 PM Michael Niedermayer > > > > <mich...@niedermayer.cc> wrote: > > > > > > > > > > On Fri, Oct 22, 2021 at 12:20:19PM +0300, Jan Ekström wrote: > > > > > > On Fri, Oct 22, 2021 at 1:13 AM Michael Niedermayer > > > > > > <mich...@niedermayer.cc> wrote: > > > > > > > > > > > > > > On Fri, Oct 22, 2021 at 12:32:18AM +0300, Jan Ekström wrote: > > > > > > > > On Fri, Oct 22, 2021 at 12:21 AM Jan Ekström <jee...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, Oct 21, 2021 at 11:49 PM Michael Niedermayer > > > > > > > > > <mich...@niedermayer.cc> wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Oct 21, 2021 at 08:48:35PM +0300, Jan Ekström wrote: > > > > > > > > > > > On Thu, Oct 21, 2021 at 8:26 PM Michael Niedermayer > > > > > > > > > > > <mich...@niedermayer.cc> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Oct 20, 2021 at 12:02:13AM +0300, Jan Ekström > > > > > > > > > > > > wrote: > > > > > > > > > > > > > On Mon, Oct 18, 2021 at 3:47 PM Jan Ekström > > > > > > > > > > > > > <jee...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Originally added as a private entry in commit > > > > > > > > > > > > > > 3f75e5116b900f1428aa13041fc7d6301bf1988a, but its > > > > > > > > > > > > > > grouping with > > > > > > > > > > > > > > the comment noting its private state was missed > > > > > > > > > > > > > > during merging of > > > > > > > > > > > > > > the field from Libav (most likely due to an already > > > > > > > > > > > > > > existing field > > > > > > > > > > > > > > in between). > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > > > > > Its a public struct in a public header so deprecate > > > > > > > > > > > > > > > > > > > > > > > > thx > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > OK, just confirming but so you don't consider this to be > > > > > > > > > > > of the same > > > > > > > > > > > type as these > > > > > > > > > > > http://git.videolan.org/?p=ffmpeg.git;a=blobdiff;f=libavformat/avio.h;h=a7b56ab6674f0a25be3b480df314e2d1292f397b;hp=0b35409787c925d681a121aa8d8715c3921fddf2;hb=45bfe8b838275235412777dd430206d9a24eb3ee;hpb=530ac6aa305aeda631c77f8a17e96c14c7ab1a1c > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > i assume this was during ABI bumping, > > > > > > > > > > when the major version numbers change then ABI/API breaking > > > > > > > > > > changes can be > > > > > > > > > > done. > > > > > > > > > > > > > > > > > > > > > > > > > > > > IIRC ABI breakage after bumping was supposed to be a ~month > > > > > > > > > but then > > > > > > > > > IIRC people posted such things much later. Was there a > > > > > > > > > message or note > > > > > > > > > when this period ended? > > > > > > > > > > > > > > Maybe someone assumed that everyone agreed what "a ~month" is ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This commit > > > > > > > > (http://git.videolan.org/?p=ffmpeg.git;a=commit;h=45bfe8b838275235412777dd430206d9a24eb3ee) > > > > > > > > was applied in late August (as the entries were private - kind > > > > > > > > of like > > > > > > > > this one remaining entry), and I can find Anton noting in May > > > > > > > > that the > > > > > > > > project is in ABI instability period. So do excuse me for not > > > > > > > > knowing > > > > > > > > which mode we are in right now if it went on for at least four > > > > > > > > months > > > > > > > > :D > > > > > > > > > > > > > > I think you should really implement a full statistics API instead > > > > > > > of > > > > > > > this as you clearly have alot of time. > > > > > > > > > > > > I do not have a lot of time, I just saw this as a finalization of > > > > > > the > > > > > > clean-up of private fields that Andreas started and I wanted to ask > > > > > > about which people would prefer, since we have not yet had a release > > > > > > branching with the API major version that this clean-up is included > > > > > > in. > > > > > > > > > > > > If you disagree with Andreas's actions from late August, then I > > > > > > would > > > > > > have preferred you to note that. I would have been OK with > > > > > > something a > > > > > > la "It was not clearly marked in our (FFmpeg's) merge so we consider > > > > > > it public and not private.", instead I got what (at least to me) > > > > > > felt > > > > > > like re-education on what API/ABI breaks are, plus what seemed to be > > > > > > not reading all those references I provided to why I thought this > > > > > > was > > > > > > a private field and why this in my opinion was just a continuation > > > > > > of > > > > > > the previous commit done by Andreas. I am sorry for the knee-jerk > > > > > > reflex-like response, it is not productive. I think I just felt like > > > > > > getting ignored and getting a standard "he doesn't understand the > > > > > > basics of this basic thing" response, even though I attempted to > > > > > > provide all the information regarding *why* I was asking. > > > > > > > > > > Well, the idea of ABI compatibility is that fields which are not > > > > > clearly marked as private cannot be removed or move around in the > > > > > binary > > > > > layout of a struct. So a user can depend on code continuing to work > > > > > A removial would violate that. > > > > > > > > Agreed. I was trying to lay before people the things I had noticed > > > > during my research into this field's history, which clearly showed > > > > that it was intended to be 100% private when added in Libav, and that > > > > it ended up being right next to private ones in FFmpeg, albeit without > > > > a clear comment on top of it due to merge conflicts. If people saw it > > > > still technically being public due to how the comment was no longer > > > > right next to the struct member, then so be it :) . > > > > > > > > Just to recap: > > > > The field was brought in with > > > > http://git.videolan.org/?p=ffmpeg.git;a=blobdiff;f=libavformat/avio.h;h=7bf7985c5e04a551e3a2841decc6b32ded7c0799;hp=49721aa1315fc44bf46d73d343ace4e7c3dc785e;hb=3f75e5116b900f1428aa13041fc7d6301bf1988a;hpb=fc85646ad495f3418042468da415af73a7a07334 > > > > (you can see the comment noting its private state on the top of the > > > > context) > > > > > > > > which then was merged as > > > > http://git.videolan.org/?p=ffmpeg.git;a=blobdiff;f=libavformat/avio.h;h=525eb7129eb5b5a5a2a8dc015275526b9b25a0cb;hp=6f4ed8440d684e03e70fd296849b76d216d6fbbb;hb=34d7f337c1608c72be8c36355018dc894f0560ce;hpb=c5fd47fa8a300fc51489a47da94041609545803c > > > > (it went just under another private struct member from the FFmpeg side) > > > > > > > > > Now the arguments i see here are > > > > > 1. There was no release > > > > > That is true but our download page first and biggest points to master > > > > > not a release. So IMHO if we point to master then master should be > > > > > working in "all circumstances" and that includes its shared libs and > > > > > ABI > > > > > > > > > > > > > True. But then instead of having major bumps and periods afterwards, > > > > we would have to properly bump major with each such change. Or work in > > > > a separate branch and then pull it into master as the Big Bump. > > > > > > > > First one is mostly something people dislike due to cosmetics (or > > > > having to move deprecations onto a higher number dynamically if a > > > > major version gets bumped multiple times in close proximity). Second > > > > one means that people are much less likely to test that branch, as > > > > well as incompatibilities when a person would be going backwards in > > > > time in the branch. That said, the latter is what we already have in > > > > master during ABI instability :) . > > > > > > > > > 2. probably noone used that field > > > > > > > > I have so far tried really hard to not utilize that argument, since at > > > > least for me it seems like to attempt to get consensus on whether a > > > > field is private or public is simpler than trying to prove that the > > > > field wasn't utilized by anyone. > > > > > > > > - If private, it can be removed. > > > > - If public, it needs deprecation. > > > > > > > > > I agree but you asked for my oppinion. > > > > > I have no strong oppinion but when you ask me what is the "proper" > > > > > thing > > > > > to do then its, all removed public fields need deprecation. > > > > > And its also less work than proofing that nothing > > > > > used a field and that its removial which may reshuffle other things > > > > > also > > > > > wont be an issue. More so as its hard to proof this given we cannot > > > > > know > > > > > all code that uses our libs. > > > > > > > > > > Theres alot that can be improved IMHO > > > > > > > > Massive yes from here on this :) I think we've noticed various things > > > > that are sub-optimal with these things. > > > > > > > > > * master during any period of instability should not allow a simple > > > > > --enable-shared and then make install without something like a extra > > > > > --allow-unstable-abi > > > > > > > > > > > > > Interesting proposition, and could possibly be matched with your later > > > > proposition with regards to a specific major version/SONAME. > > > > > > > > > * periods of ABI/API instability should be short and better > > > > > coordinated > > > > > people should arrange their planned changes before and not keep > > > > > extending > > > > > these periods > > > > > > > > Definitely. The question is how we could make queuing these things > > > > easier for people. On a merge request style platform I'd probably see > > > > this handled by tags/flags that mark that those open change sets > > > > should be handled at the next bump (tagging lets one easily either > > > > filter the view of change sets for just those, or excluding those). > > > > Then if a bump is announced (let's say a month or two before it > > > > happens, so that people can get their change sets ready for rebase), > > > > reviews are started so that when a bump happens, it's swift and done > > > > with. > > > > > > > > > > > > > > * maybe we should use a special ABI version for these unstable period > > > > > that way half of the problems disappear, its then clear when that > > > > > period > > > > > ends. And its not possible to install that under a stable ABI/API > > > > > soname > > > > > > > > > > > > > That's an interesting proposition. Just not sure what's the best way > > > > for this? Negative or very large SONAME? > > > > > > > > > * maybe our download page should not just point to a master snapshot > > > > > when > > > > > our ABI/API is in a transition period > > > > > > > > > > so, if you want to just remove the field, sure do it it probably will > > > > > be > > > > > fine but the proper way from ABI/API compatibility POV is to > > > > > deprecate it > > > > > > > > > > > > > So just to bring this set to the finish line, do I understand that for > > > > you the field was not well enough flagged as private, and should thus > > > > be treated as a public struct member? > > > > > > > > I would prefer removal as I've looked into the history of the field > > > > and I saw Andreas working hard on removing the private entries from > > > > this struct, but technically I can see that due to the Libav->FFmpeg > > > > merge breaking the connection it could be argued that the field was > > > > not flagged as private when added to FFmpeg. Which is why Andreas most > > > > likely missed it from the struct in his clean-up commit. > > > > > > > > If you think it is public, then I will just go with this version of > > > > the patch set, including deprecation. For me the main thing was to > > > > highlight and make people think whether this struct member was meant > > > > as public or private, and then decide how it should be interpreted in > > > > current FFmpeg master. Poking James on IRC, he seems to be more > > > > towards removal when looking at it in the context of how and where it > > > > was added, but also first noted that deprecation would be the cleanest > > > > way to remove it. > > > > > > why would one think its private ? > > > > > > The struct documentation says this: > > > /** > > > * Bytestream IO Context. > > > * New public fields can be added with minor version bumps. > > > * Removal, reordering and changes to existing public fields require > > > * a major version bump. > > > * sizeof(AVIOContext) must not be used outside libav*. > > > * > > > * @note None of the function pointers in AVIOContext should be called > > > * directly, they should only be set by the client application > > > * when implementing custom I/O. Normally these are set to the > > > * function pointers specified in avio_alloc_context() > > > */ > > > > > > > > > This speaks only of public fields > > > it also excludes the function pointers, but nothing else is excluded > > > > > > and then theres > > > > > > int64_t written; > > > > > > why would this not be a public field ? > > > what is the reasoning behind that? > > > > > > > Because I thought the how it was meant to be and how close in context > > it was to groups of private members would add some value when making > > the decision of whether to put it into one group of struct members or > > into another. Thus at the very least I thought it was worth bringing > > it up and hearing opinions. > > > > Until very recently the region of the struct would be more like > > > > /** > > * Internal, not meant to be used from outside of AVIOContext. > > */ > > enum AVIODataMarkerType current_type; > > int64_t last_time; > > > > /** > > * A callback that is used instead of short_seek_threshold. > > * This is current internal only, do not use from outside. > > */ > > int (*short_seek_get)(void *opaque); > > > > int64_t written; > > > > /** > > * Maximum reached position before a backward seek in the write buffer, > > * used keeping track of already written data for a later flush. > > */ > > unsigned char *buf_ptr_max; > > > > /** > > * Try to buffer at least this amount of data before flushing it > > */ > > int min_packet_size; > > > > If you strictly read only into what's in this context, it is an > > undocumented member near some internal ones. > > > > But sure, it seems like I am not going to get a straight answer here, > > for whatever reason. I will just take this as the implied "it is > > private in my opinion". Which is fine. It was one of the alternatives > > I noted, and a valid reading based on the lack of comments stuck right > > next to the field. > > I meant, "it is public in my opinion" here, of course. Sorry. Kind of > missed I had typed the wrong words while going through thoughts.
And here in shame I notice that this indeed was your initial response. OK, sorry about that. Somehow I had gotten swept into this so I had forgotten about it until I scrolled through the patchwork comment thread again. This is what I guess being tired does to one, trying to request an answer to finalize a thread while that answer was already there in the beginning. Jan _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
