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

Fri Oct 22 23:25:46 EEST 2021

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?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211022/49bf686f/attachment.sig>