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? You can also poll maybe 3 developers unrelated to FFmpeg and not knowing your preferrance or the codebase. "Is this a public field an application can use if it needs to ?" If someone says "No" then iam really currious why someone came to this conclusion. I dont think anyone would investigate the git log of when a field was added or other related cleanups. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato
signature.asc
Description: PGP signature
_______________________________________________ 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".
