[FFmpeg-devel] [PATCH v3 2/3] avformat/avio{, buf}: deprecate AVIOContext::written

Jan Ekström jeebjp at gmail.com
Sat Oct 23 00:13:00 EEST 2021


On Sat, Oct 23, 2021 at 12:11 AM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Fri, Oct 22, 2021 at 11:26 PM Michael Niedermayer
> <michael at 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
> > > <michael at 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
> > > > > <michael at 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 <jeebjp at gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 21, 2021 at 11:49 PM Michael Niedermayer
> > > > > > > > <michael at 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
> > > > > > > > > > <michael at 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 <jeebjp at 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.

Jan


More information about the ffmpeg-devel mailing list