[FFmpeg-devel] avcodec: Require avoptions for the user to set max_pixels.

Submitted by Michael Niedermayer on Dec. 10, 2016, 10:01 p.m.

Details

Message ID 20161210220104.14461-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 10, 2016, 10:01 p.m.
When we will backport this, it will be inevitably in a different location
in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
use the same soname though and must have a binary compatible interface.
It thus can only saftely be accessed through AVOptions

It may be enough to require this only in the releases but that could be
rather confusing.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avcodec.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

James Almer Dec. 10, 2016, 11:31 p.m.
On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> When we will backport this, it will be inevitably in a different location
> in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> use the same soname though and must have a binary compatible interface.
> It thus can only saftely be accessed through AVOptions
> 
> It may be enough to require this only in the releases but that could be
> rather confusing.

Wouldn't it be better to initially add such new fields inside
AVCodecContext->internal and accessible by AVOptions only so they
may be trivial to backport, then once a major bump occurs they can
be moved to the actual public struct with enabled direct access?

Not sure how feasible is to have options_table.h options change
values from that internal struct, but it would solve the above
concerns.

> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avcodec.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 02234aee67..8123092ac0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
>      /**
>       * The number of pixels per image to maximally accept.
>       *
> -     * - decoding: set by user
> -     * - encoding: set by user
> +     * - decoding: set by user through AVOptions (NO direct access)
> +     * - encoding: set by user through AVOptions (NO direct access)
>       */
>      int64_t max_pixels;
>  
>
Michael Niedermayer Dec. 11, 2016, 12:23 a.m.
On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> > When we will backport this, it will be inevitably in a different location
> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > use the same soname though and must have a binary compatible interface.
> > It thus can only saftely be accessed through AVOptions
> > 
> > It may be enough to require this only in the releases but that could be
> > rather confusing.
> 
> Wouldn't it be better to initially add such new fields inside
> AVCodecContext->internal and accessible by AVOptions only so they
> may be trivial to backport, then once a major bump occurs they can
> be moved to the actual public struct with enabled direct access?
> 
> Not sure how feasible is to have options_table.h options change
> values from that internal struct, but it would solve the above
> concerns.

AVCodecInternal does not start with a AVClass / AVOption table
(thats solvable though would need to be done to past releases)

AVCodecContext->internal is not a child class of AVCodecContext
(that is solvable too, if we want to take the risk to do that in
 point releases)

AVCodecContext->internal is allocated in avcodec_open2() so nothing
can be set in it by the user before
(i dont see a solution to this that we would want to backport)

If you suggest to make these changes now to for future use.
that will make backports across this change very annoying, as theres
a point before AVCodecInternal cannot be used.
And also theres more work for us to maintain seperate implementations
for the options, all code accessing them has to deal with them being
in different places, making all related backports harder.

To me that looks like a disadvantage from every side

I think the real solution is to start liking AVOptions, they make
our work easier.

[...]
Michael Niedermayer Dec. 11, 2016, 12:38 a.m.
On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> > When we will backport this, it will be inevitably in a different location
> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > use the same soname though and must have a binary compatible interface.
> > It thus can only saftely be accessed through AVOptions
> > 
> > It may be enough to require this only in the releases but that could be
> > rather confusing.
> 
> Wouldn't it be better to initially add such new fields inside
> AVCodecContext->internal and accessible by AVOptions only so they
> may be trivial to backport, then once a major bump occurs they can
> be moved to the actual public struct with enabled direct access?
> 
> Not sure how feasible is to have options_table.h options change
> values from that internal struct, but it would solve the above
> concerns.
[...]

If people want we can write something like:

    /**
     * The number of pixels per image to maximally accept.
     *
     * - decoding: set by user through AVOptions (NO direct access before 57.67.100)
     * - encoding: set by user through AVOptions (NO direct access before 57.67.100)
     */
    int64_t max_pixels;

This would allow direct access in the future and require AVOptions in
releases but its quite complex


[...]
James Almer Dec. 11, 2016, 12:52 a.m.
On 12/10/2016 9:23 PM, Michael Niedermayer wrote:
> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
>>> When we will backport this, it will be inevitably in a different location
>>> in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
>>> use the same soname though and must have a binary compatible interface.
>>> It thus can only saftely be accessed through AVOptions
>>>
>>> It may be enough to require this only in the releases but that could be
>>> rather confusing.
>>
>> Wouldn't it be better to initially add such new fields inside
>> AVCodecContext->internal and accessible by AVOptions only so they
>> may be trivial to backport, then once a major bump occurs they can
>> be moved to the actual public struct with enabled direct access?
>>
>> Not sure how feasible is to have options_table.h options change
>> values from that internal struct, but it would solve the above
>> concerns.
> 
> AVCodecInternal does not start with a AVClass / AVOption table
> (thats solvable though would need to be done to past releases)
> 
> AVCodecContext->internal is not a child class of AVCodecContext
> (that is solvable too, if we want to take the risk to do that in
>  point releases)
> 
> AVCodecContext->internal is allocated in avcodec_open2() so nothing
> can be set in it by the user before
> (i dont see a solution to this that we would want to backport)
> 
> If you suggest to make these changes now to for future use.
> that will make backports across this change very annoying, as theres
> a point before AVCodecInternal cannot be used.

If this is impossible now for the above reasons then it's something
that could be considered for the next bump, or the one after that
if it requires careful planning and design.
I was simply suggesting something that could prevent backporting
fields on public structs that would not have a fixed offset.

The plan was to undo all the current ones in the next bump now that
we dropped ABI compatibility with Libav, so it would be nice to find
a way to avoid something like that happening again.

> And also theres more work for us to maintain seperate implementations
> for the options, all code accessing them has to deal with them being
> in different places, making all related backports harder.
> 
> To me that looks like a disadvantage from every side
> 
> I think the real solution is to start liking AVOptions, they make
> our work easier.

AVOptions are fine. Private-but-not-really and no-direct-access fields
in public structs are what's kinda ugly an unpopular.

No objections or other comments for this patch from me.
wm4 Dec. 11, 2016, 12:54 p.m.
On Sat, 10 Dec 2016 23:01:04 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> When we will backport this, it will be inevitably in a different location
> in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> use the same soname though and must have a binary compatible interface.
> It thus can only saftely be accessed through AVOptions
> 
> It may be enough to require this only in the releases but that could be
> rather confusing.
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avcodec.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 02234aee67..8123092ac0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
>      /**
>       * The number of pixels per image to maximally accept.
>       *
> -     * - decoding: set by user
> -     * - encoding: set by user
> +     * - decoding: set by user through AVOptions (NO direct access)
> +     * - encoding: set by user through AVOptions (NO direct access)
>       */
>      int64_t max_pixels;
>  

Why? We gave up the Libav ABI compat. abomination.
Michael Niedermayer Dec. 11, 2016, 2:59 p.m.
On Sun, Dec 11, 2016 at 01:54:28PM +0100, wm4 wrote:
> On Sat, 10 Dec 2016 23:01:04 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > When we will backport this, it will be inevitably in a different location
> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > use the same soname though and must have a binary compatible interface.
> > It thus can only saftely be accessed through AVOptions
> > 
> > It may be enough to require this only in the releases but that could be
> > rather confusing.
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avcodec.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 02234aee67..8123092ac0 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
> >      /**
> >       * The number of pixels per image to maximally accept.
> >       *
> > -     * - decoding: set by user
> > -     * - encoding: set by user
> > +     * - decoding: set by user through AVOptions (NO direct access)
> > +     * - encoding: set by user through AVOptions (NO direct access)
> >       */
> >      int64_t max_pixels;
> >  
> 
> Why? We gave up the Libav ABI compat. abomination.

Its explained in the patch comment above

max_pixels should to be backported as it allows users to control memory
use from large images better, avoid some OOMs and fixes issues which
some people consider security bugs
if its backported it will not be in the same location relative to the
start of AVCodecContext in master, 3.2, 3.1, 3.0
master, 3.2, 3.1, 3.0 all have the same soname
libs using the same soname need to be binary compatible
direct access to one location will not work and thus be binary
incompatible if the field is at a different location

Thus releases cannot use direct access to the max_pixels field.
One could say it differently in that if 3.0.X would access max_pixels
directly at byte offset 123 then an application built against that
and linked to 3.2 will access another field before max_pixels in 3.2

AVOptions solves that corner case of backporting added fields.
This restriction can simply be droped on the next ABI major bump

[...]
Hendrik Leppkes Dec. 11, 2016, 3:02 p.m.
On Sun, Dec 11, 2016 at 3:59 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Sun, Dec 11, 2016 at 01:54:28PM +0100, wm4 wrote:
>> On Sat, 10 Dec 2016 23:01:04 +0100
>> Michael Niedermayer <michael@niedermayer.cc> wrote:
>>
>> > When we will backport this, it will be inevitably in a different location
>> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
>> > use the same soname though and must have a binary compatible interface.
>> > It thus can only saftely be accessed through AVOptions
>> >
>> > It may be enough to require this only in the releases but that could be
>> > rather confusing.
>> >
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavcodec/avcodec.h | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> > index 02234aee67..8123092ac0 100644
>> > --- a/libavcodec/avcodec.h
>> > +++ b/libavcodec/avcodec.h
>> > @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
>> >      /**
>> >       * The number of pixels per image to maximally accept.
>> >       *
>> > -     * - decoding: set by user
>> > -     * - encoding: set by user
>> > +     * - decoding: set by user through AVOptions (NO direct access)
>> > +     * - encoding: set by user through AVOptions (NO direct access)
>> >       */
>> >      int64_t max_pixels;
>> >
>>
>> Why? We gave up the Libav ABI compat. abomination.
>
> Its explained in the patch comment above
>
> max_pixels should to be backported as it allows users to control memory
> use from large images better, avoid some OOMs and fixes issues which
> some people consider security bugs
> if its backported it will not be in the same location relative to the
> start of AVCodecContext in master, 3.2, 3.1, 3.0
> master, 3.2, 3.1, 3.0 all have the same soname
> libs using the same soname need to be binary compatible
> direct access to one location will not work and thus be binary
> incompatible if the field is at a different location
>
> Thus releases cannot use direct access to the max_pixels field.
> One could say it differently in that if 3.0.X would access max_pixels
> directly at byte offset 123 then an application built against that
> and linked to 3.2 will access another field before max_pixels in 3.2
>

Thats why one doesn't backport features. It just causes headaches when
structs change.

- Hendrik
Nicolas George Dec. 11, 2016, 3:06 p.m.
Le primidi 21 frimaire, an CCXXV, Michael Niedermayer a écrit :
> Its explained in the patch comment above
> 
> max_pixels should to be backported as it allows users to control memory
> use from large images better, avoid some OOMs and fixes issues which
> some people consider security bugs
> if its backported it will not be in the same location relative to the
> start of AVCodecContext in master, 3.2, 3.1, 3.0
> master, 3.2, 3.1, 3.0 all have the same soname
> libs using the same soname need to be binary compatible
> direct access to one location will not work and thus be binary
> incompatible if the field is at a different location

I think this is a terrible reason to make the code needlessly more
complicated.

I do not know where this new hype of treating OOM as a security issue
comes from, but if we go in that direction it will take much more than a
few ill-thought options to fix it.

OOM is not a security issue. If people want to avoid it, let them use
their operating system's features. And if there are other security
issues that this is supposed to fix, document them before proposing a
shaky fix.

And please do not clutter the code for the years to come like that.

Regards,
Michael Niedermayer Dec. 11, 2016, 3:06 p.m.
On Sun, Dec 11, 2016 at 04:02:27PM +0100, Hendrik Leppkes wrote:
> On Sun, Dec 11, 2016 at 3:59 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Sun, Dec 11, 2016 at 01:54:28PM +0100, wm4 wrote:
> >> On Sat, 10 Dec 2016 23:01:04 +0100
> >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>
> >> > When we will backport this, it will be inevitably in a different location
> >> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> >> > use the same soname though and must have a binary compatible interface.
> >> > It thus can only saftely be accessed through AVOptions
> >> >
> >> > It may be enough to require this only in the releases but that could be
> >> > rather confusing.
> >> >
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  libavcodec/avcodec.h | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> >> > index 02234aee67..8123092ac0 100644
> >> > --- a/libavcodec/avcodec.h
> >> > +++ b/libavcodec/avcodec.h
> >> > @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
> >> >      /**
> >> >       * The number of pixels per image to maximally accept.
> >> >       *
> >> > -     * - decoding: set by user
> >> > -     * - encoding: set by user
> >> > +     * - decoding: set by user through AVOptions (NO direct access)
> >> > +     * - encoding: set by user through AVOptions (NO direct access)
> >> >       */
> >> >      int64_t max_pixels;
> >> >
> >>
> >> Why? We gave up the Libav ABI compat. abomination.
> >
> > Its explained in the patch comment above
> >
> > max_pixels should to be backported as it allows users to control memory
> > use from large images better, avoid some OOMs and fixes issues which
> > some people consider security bugs
> > if its backported it will not be in the same location relative to the
> > start of AVCodecContext in master, 3.2, 3.1, 3.0
> > master, 3.2, 3.1, 3.0 all have the same soname
> > libs using the same soname need to be binary compatible
> > direct access to one location will not work and thus be binary
> > incompatible if the field is at a different location
> >
> > Thus releases cannot use direct access to the max_pixels field.
> > One could say it differently in that if 3.0.X would access max_pixels
> > directly at byte offset 123 then an application built against that
> > and linked to 3.2 will access another field before max_pixels in 3.2
> >
> 
> Thats why one doesn't backport features. It just causes headaches when
> structs change.

Security fixes require such changes sometimes

the whitelists, if someone did the rather massive work to backport
them would be similar

[...]
wm4 Dec. 11, 2016, 3:14 p.m.
On Sun, 11 Dec 2016 16:06:41 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Sun, Dec 11, 2016 at 04:02:27PM +0100, Hendrik Leppkes wrote:
> > On Sun, Dec 11, 2016 at 3:59 PM, Michael Niedermayer
> > <michael@niedermayer.cc> wrote:  
> > > On Sun, Dec 11, 2016 at 01:54:28PM +0100, wm4 wrote:  
> > >> On Sat, 10 Dec 2016 23:01:04 +0100
> > >> Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >>  
> > >> > When we will backport this, it will be inevitably in a different location
> > >> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > >> > use the same soname though and must have a binary compatible interface.
> > >> > It thus can only saftely be accessed through AVOptions
> > >> >
> > >> > It may be enough to require this only in the releases but that could be
> > >> > rather confusing.
> > >> >
> > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >> > ---
> > >> >  libavcodec/avcodec.h | 4 ++--
> > >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > >> > index 02234aee67..8123092ac0 100644
> > >> > --- a/libavcodec/avcodec.h
> > >> > +++ b/libavcodec/avcodec.h
> > >> > @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
> > >> >      /**
> > >> >       * The number of pixels per image to maximally accept.
> > >> >       *
> > >> > -     * - decoding: set by user
> > >> > -     * - encoding: set by user
> > >> > +     * - decoding: set by user through AVOptions (NO direct access)
> > >> > +     * - encoding: set by user through AVOptions (NO direct access)
> > >> >       */
> > >> >      int64_t max_pixels;
> > >> >  
> > >>
> > >> Why? We gave up the Libav ABI compat. abomination.  
> > >
> > > Its explained in the patch comment above
> > >
> > > max_pixels should to be backported as it allows users to control memory
> > > use from large images better, avoid some OOMs and fixes issues which
> > > some people consider security bugs
> > > if its backported it will not be in the same location relative to the
> > > start of AVCodecContext in master, 3.2, 3.1, 3.0
> > > master, 3.2, 3.1, 3.0 all have the same soname
> > > libs using the same soname need to be binary compatible
> > > direct access to one location will not work and thus be binary
> > > incompatible if the field is at a different location
> > >
> > > Thus releases cannot use direct access to the max_pixels field.
> > > One could say it differently in that if 3.0.X would access max_pixels
> > > directly at byte offset 123 then an application built against that
> > > and linked to 3.2 will access another field before max_pixels in 3.2
> > >  
> > 
> > Thats why one doesn't backport features. It just causes headaches when
> > structs change.  
> 
> Security fixes require such changes sometimes
> 
> the whitelists, if someone did the rather massive work to backport
> them would be similar
> 
> [...]

It's not a security fix. It merely triggers OOM, and there are
literally thousands of other things that can trigger OOM.

Let me repeat: calling max_pixels a security fix is 100% bullshit.
Michael Niedermayer Dec. 11, 2016, 4:19 p.m.
On Sun, Dec 11, 2016 at 04:06:00PM +0100, Nicolas George wrote:
> Le primidi 21 frimaire, an CCXXV, Michael Niedermayer a écrit :
> > Its explained in the patch comment above
> > 
> > max_pixels should to be backported as it allows users to control memory
> > use from large images better, avoid some OOMs and fixes issues which
> > some people consider security bugs
> > if its backported it will not be in the same location relative to the
> > start of AVCodecContext in master, 3.2, 3.1, 3.0
> > master, 3.2, 3.1, 3.0 all have the same soname
> > libs using the same soname need to be binary compatible
> > direct access to one location will not work and thus be binary
> > incompatible if the field is at a different location
> 
> I think this is a terrible reason to make the code needlessly more
> complicated.
> 
> I do not know where this new hype of treating OOM as a security issue
> comes from, but if we go in that direction it will take much more than a
> few ill-thought options to fix it.
> 
> OOM is not a security issue. If people want to avoid it, let them use
> their operating system's features. And if there are other security
> issues that this is supposed to fix, document them before proposing a
> shaky fix.

As long as it is not documented that you need to run libavcodec/format
in a seperate process it is a security issue if you crash.
remotely triggered crashes are a security issue, this is not new.

iam not really in the camp that belives that these OOM crashes are
a real security issue nor am i in the camp that belives they are non
issues. (i in fact had pointed some security researchers who reported
some OOM issues to threads here previously and the effect of that
was that they simply registered CVE# for the issues and published
them (after some time) not bothering to inform me about either)
i had hoped they would join the discussions ...

One part of me can understand that, one part of me can understand the
community but i do not enjoy being between the 2 at all and i kind of
am.

What id like to do is simply fix the issues i can fix. The max_pixels
and max_streams code is doing that.

Also i think people should make up their mind.
If you (plural) want to make it mandatory to run libavcodec/format in
a seperate process if the input is untrusted you should have a priority
of documenting that, and think about what it means for past releases
that did not have such a requirment documented


And compltely indepandant of the security aspect, the pixels and
streams are special in that they can cause OOM crashes without being
crafted input files, in fact totally valid files can trigger it.

And i disagree that not crashing on valid files is a feature.
(one argument against backporting seemed to be that its a feature and
 not a bugfix)


[...]
Nicolas George Dec. 11, 2016, 4:29 p.m.
Le primidi 21 frimaire, an CCXXV, Michael Niedermayer a écrit :
> As long as it is not documented that you need to run libavcodec/format
> in a seperate process it is a security issue if you crash.

This is not specific to FFmpeg and documented in books and courses on
development in general.

> iam not really in the camp that belives that these OOM crashes are
> a real security issue nor am i in the camp that belives they are non
> issues. (i in fact had pointed some security researchers who reported
> some OOM issues to threads here previously and the effect of that
> was that they simply registered CVE# for the issues and published
> them (after some time) not bothering to inform me about either)
> i had hoped they would join the discussions ...

The short summary you give here makes it look that these so-called
security "researchers" are just idiots.

> What id like to do is simply fix the issues i can fix. The max_pixels
> and max_streams code is doing that.

No, they do not. They mitigate a few corner cases at the cost of
complexity and brittleness.

> And compltely indepandant of the security aspect, the pixels and
> streams are special in that they can cause OOM crashes without being
> crafted input files, in fact totally valid files can trigger it.

Which is another clue that OOM is not a security issue in itself.

My opinion:

Revert all this while we still can just do it without extra work.

Regards,
Michael Niedermayer Dec. 12, 2016, 1:29 a.m.
On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:
> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:
> > On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> >> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
[...]

> > And also theres more work for us to maintain seperate implementations
> > for the options, all code accessing them has to deal with them being
> > in different places, making all related backports harder.
> > 
> > To me that looks like a disadvantage from every side
> > 
> > I think the real solution is to start liking AVOptions, they make
> > our work easier.
> 
> AVOptions are fine. Private-but-not-really and no-direct-access fields
> in public structs are what's kinda ugly an unpopular.

a slightly off topic question but
if people care about these existing "no direct access" fields
why do they not change them ?
its a bit reading and thinking and droping the "no direct access"
comments, this is not much work
It requires a tiny bit of care so that future added fields dont do
bad things and we should document that past releases still in some
cases need the indirect access  ...

just seems a bit odd to me in relation to the opposition to add such
a field were its needed for a bugfix but total apparent lack of
interrest in removing such "no direct access" restrctions  where there
is no reason at all to keep them

[...]
James Almer Dec. 12, 2016, 1:51 a.m.
On 12/11/2016 10:29 PM, Michael Niedermayer wrote:
> On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:
>> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:
>>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
>>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> [...]
> 
>>> And also theres more work for us to maintain seperate implementations
>>> for the options, all code accessing them has to deal with them being
>>> in different places, making all related backports harder.
>>>
>>> To me that looks like a disadvantage from every side
>>>
>>> I think the real solution is to start liking AVOptions, they make
>>> our work easier.
>>
>> AVOptions are fine. Private-but-not-really and no-direct-access fields
>> in public structs are what's kinda ugly an unpopular.
> 
> a slightly off topic question but
> if people care about these existing "no direct access" fields
> why do they not change them ?
> its a bit reading and thinking and droping the "no direct access"
> comments, this is not much work
> It requires a tiny bit of care so that future added fields dont do
> bad things and we should document that past releases still in some
> cases need the indirect access  ...
> 
> just seems a bit odd to me in relation to the opposition to add such
> a field were its needed for a bugfix but total apparent lack of
> interrest in removing such "no direct access" restrctions  where there
> is no reason at all to keep them

They are all supposed to stop being no direct access with the next bump.
Afaik most are remnants of the libav ABI compatibility days, same with
the get/setter functions, but they can't just be removed right now because
there are several versions using the current ABI (3.0 and newer i think).

Once that's done, any new field, be it ours or merged from libav, will
always go either at the end or below the "Internal from here on" marker,
assuming we don't get rid of those as well.
Michael Niedermayer Dec. 12, 2016, 2:34 a.m.
On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:
> On 12/11/2016 10:29 PM, Michael Niedermayer wrote:
> > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:
> >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:
> >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> > [...]
> > 
> >>> And also theres more work for us to maintain seperate implementations
> >>> for the options, all code accessing them has to deal with them being
> >>> in different places, making all related backports harder.
> >>>
> >>> To me that looks like a disadvantage from every side
> >>>
> >>> I think the real solution is to start liking AVOptions, they make
> >>> our work easier.
> >>
> >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> >> in public structs are what's kinda ugly an unpopular.
> > 
> > a slightly off topic question but
> > if people care about these existing "no direct access" fields
> > why do they not change them ?
> > its a bit reading and thinking and droping the "no direct access"
> > comments, this is not much work
> > It requires a tiny bit of care so that future added fields dont do
> > bad things and we should document that past releases still in some
> > cases need the indirect access  ...
> > 
> > just seems a bit odd to me in relation to the opposition to add such
> > a field were its needed for a bugfix but total apparent lack of
> > interrest in removing such "no direct access" restrctions  where there
> > is no reason at all to keep them
> 
> They are all supposed to stop being no direct access with the next bump.
> Afaik most are remnants of the libav ABI compatibility days, same with
> the get/setter functions, but they can't just be removed right now because
> there are several versions using the current ABI (3.0 and newer i think).

theres no need to wait for the next bump with removing many of the
"no direct access" rules because the fields always where in the same
position in the current major version

Removing them before the bump would allow people to stop using
the accessors earlier

someone needs to read through the struct and think about it though
so nothing is missed but from just the diff between master and
the releases there seem to be many that did not move


> 
> Once that's done, any new field, be it ours or merged from libav, will
> always go either at the end or below the "Internal from here on" marker,
> assuming we don't get rid of those as well.
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Dec. 12, 2016, 8:48 a.m.
On Mon, 12 Dec 2016 03:34:27 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:
> > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:  
> > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:  
> > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:  
> > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:  
> > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:  
> > > [...]
> > >   
> > >>> And also theres more work for us to maintain seperate implementations
> > >>> for the options, all code accessing them has to deal with them being
> > >>> in different places, making all related backports harder.
> > >>>
> > >>> To me that looks like a disadvantage from every side
> > >>>
> > >>> I think the real solution is to start liking AVOptions, they make
> > >>> our work easier.  
> > >>
> > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > >> in public structs are what's kinda ugly an unpopular.  
> > > 
> > > a slightly off topic question but
> > > if people care about these existing "no direct access" fields
> > > why do they not change them ?
> > > its a bit reading and thinking and droping the "no direct access"
> > > comments, this is not much work
> > > It requires a tiny bit of care so that future added fields dont do
> > > bad things and we should document that past releases still in some
> > > cases need the indirect access  ...
> > > 
> > > just seems a bit odd to me in relation to the opposition to add such
> > > a field were its needed for a bugfix but total apparent lack of
> > > interrest in removing such "no direct access" restrctions  where there
> > > is no reason at all to keep them  
> > 
> > They are all supposed to stop being no direct access with the next bump.
> > Afaik most are remnants of the libav ABI compatibility days, same with
> > the get/setter functions, but they can't just be removed right now because
> > there are several versions using the current ABI (3.0 and newer i think).  
> 
> theres no need to wait for the next bump with removing many of the
> "no direct access" rules because the fields always where in the same
> position in the current major version
> 
> Removing them before the bump would allow people to stop using
> the accessors earlier
> 
> someone needs to read through the struct and think about it though
> so nothing is missed but from just the diff between master and
> the releases there seem to be many that did not move

Can I send a patch that makes them "real" public fields, and which
deprecates the accessors? In all libs?
Michael Niedermayer Dec. 12, 2016, 4:34 p.m.
On Mon, Dec 12, 2016 at 09:48:01AM +0100, wm4 wrote:
> On Mon, 12 Dec 2016 03:34:27 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:
> > > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:  
> > > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:  
> > > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:  
> > > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:  
> > > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:  
> > > > [...]
> > > >   
> > > >>> And also theres more work for us to maintain seperate implementations
> > > >>> for the options, all code accessing them has to deal with them being
> > > >>> in different places, making all related backports harder.
> > > >>>
> > > >>> To me that looks like a disadvantage from every side
> > > >>>
> > > >>> I think the real solution is to start liking AVOptions, they make
> > > >>> our work easier.  
> > > >>
> > > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > > >> in public structs are what's kinda ugly an unpopular.  
> > > > 
> > > > a slightly off topic question but
> > > > if people care about these existing "no direct access" fields
> > > > why do they not change them ?
> > > > its a bit reading and thinking and droping the "no direct access"
> > > > comments, this is not much work
> > > > It requires a tiny bit of care so that future added fields dont do
> > > > bad things and we should document that past releases still in some
> > > > cases need the indirect access  ...
> > > > 
> > > > just seems a bit odd to me in relation to the opposition to add such
> > > > a field were its needed for a bugfix but total apparent lack of
> > > > interrest in removing such "no direct access" restrctions  where there
> > > > is no reason at all to keep them  
> > > 
> > > They are all supposed to stop being no direct access with the next bump.
> > > Afaik most are remnants of the libav ABI compatibility days, same with
> > > the get/setter functions, but they can't just be removed right now because
> > > there are several versions using the current ABI (3.0 and newer i think).  
> > 
> > theres no need to wait for the next bump with removing many of the
> > "no direct access" rules because the fields always where in the same
> > position in the current major version
> > 
> > Removing them before the bump would allow people to stop using
> > the accessors earlier
> > 
> > someone needs to read through the struct and think about it though
> > so nothing is missed but from just the diff between master and
> > the releases there seem to be many that did not move
> 
> Can I send a patch that makes them "real" public fields, and which
> deprecates the accessors? In all libs?

If someone first checks that they are in the same offset in their
structs in all FFmpeg releases with the same soname.
And someone checks that we have no "insert new ... field above/below
this line" things above them

Yes, definitly


[...]
wm4 Dec. 12, 2016, 4:59 p.m.
On Mon, 12 Dec 2016 17:34:15 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Dec 12, 2016 at 09:48:01AM +0100, wm4 wrote:
> > On Mon, 12 Dec 2016 03:34:27 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:  
> > > > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:    
> > > > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:    
> > > > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:    
> > > > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:    
> > > > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:    
> > > > > [...]
> > > > >     
> > > > >>> And also theres more work for us to maintain seperate implementations
> > > > >>> for the options, all code accessing them has to deal with them being
> > > > >>> in different places, making all related backports harder.
> > > > >>>
> > > > >>> To me that looks like a disadvantage from every side
> > > > >>>
> > > > >>> I think the real solution is to start liking AVOptions, they make
> > > > >>> our work easier.    
> > > > >>
> > > > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > > > >> in public structs are what's kinda ugly an unpopular.    
> > > > > 
> > > > > a slightly off topic question but
> > > > > if people care about these existing "no direct access" fields
> > > > > why do they not change them ?
> > > > > its a bit reading and thinking and droping the "no direct access"
> > > > > comments, this is not much work
> > > > > It requires a tiny bit of care so that future added fields dont do
> > > > > bad things and we should document that past releases still in some
> > > > > cases need the indirect access  ...
> > > > > 
> > > > > just seems a bit odd to me in relation to the opposition to add such
> > > > > a field were its needed for a bugfix but total apparent lack of
> > > > > interrest in removing such "no direct access" restrctions  where there
> > > > > is no reason at all to keep them    
> > > > 
> > > > They are all supposed to stop being no direct access with the next bump.
> > > > Afaik most are remnants of the libav ABI compatibility days, same with
> > > > the get/setter functions, but they can't just be removed right now because
> > > > there are several versions using the current ABI (3.0 and newer i think).    
> > > 
> > > theres no need to wait for the next bump with removing many of the
> > > "no direct access" rules because the fields always where in the same
> > > position in the current major version
> > > 
> > > Removing them before the bump would allow people to stop using
> > > the accessors earlier
> > > 
> > > someone needs to read through the struct and think about it though
> > > so nothing is missed but from just the diff between master and
> > > the releases there seem to be many that did not move  
> > 
> > Can I send a patch that makes them "real" public fields, and which
> > deprecates the accessors? In all libs?  
> 
> If someone first checks that they are in the same offset in their
> structs in all FFmpeg releases with the same soname.
> And someone checks that we have no "insert new ... field above/below
> this line" things above them
> 
> Yes, definitly

The offsets wouldn't have to be the same, since they were allowed to be
accessed by accessor only.
Michael Niedermayer Dec. 12, 2016, 6:55 p.m.
On Mon, Dec 12, 2016 at 05:59:32PM +0100, wm4 wrote:
> On Mon, 12 Dec 2016 17:34:15 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Mon, Dec 12, 2016 at 09:48:01AM +0100, wm4 wrote:
> > > On Mon, 12 Dec 2016 03:34:27 +0100
> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >   
> > > > On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:  
> > > > > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:    
> > > > > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:    
> > > > > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:    
> > > > > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:    
> > > > > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:    
> > > > > > [...]
> > > > > >     
> > > > > >>> And also theres more work for us to maintain seperate implementations
> > > > > >>> for the options, all code accessing them has to deal with them being
> > > > > >>> in different places, making all related backports harder.
> > > > > >>>
> > > > > >>> To me that looks like a disadvantage from every side
> > > > > >>>
> > > > > >>> I think the real solution is to start liking AVOptions, they make
> > > > > >>> our work easier.    
> > > > > >>
> > > > > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > > > > >> in public structs are what's kinda ugly an unpopular.    
> > > > > > 
> > > > > > a slightly off topic question but
> > > > > > if people care about these existing "no direct access" fields
> > > > > > why do they not change them ?
> > > > > > its a bit reading and thinking and droping the "no direct access"
> > > > > > comments, this is not much work
> > > > > > It requires a tiny bit of care so that future added fields dont do
> > > > > > bad things and we should document that past releases still in some
> > > > > > cases need the indirect access  ...
> > > > > > 
> > > > > > just seems a bit odd to me in relation to the opposition to add such
> > > > > > a field were its needed for a bugfix but total apparent lack of
> > > > > > interrest in removing such "no direct access" restrctions  where there
> > > > > > is no reason at all to keep them    
> > > > > 
> > > > > They are all supposed to stop being no direct access with the next bump.
> > > > > Afaik most are remnants of the libav ABI compatibility days, same with
> > > > > the get/setter functions, but they can't just be removed right now because
> > > > > there are several versions using the current ABI (3.0 and newer i think).    
> > > > 
> > > > theres no need to wait for the next bump with removing many of the
> > > > "no direct access" rules because the fields always where in the same
> > > > position in the current major version
> > > > 
> > > > Removing them before the bump would allow people to stop using
> > > > the accessors earlier
> > > > 
> > > > someone needs to read through the struct and think about it though
> > > > so nothing is missed but from just the diff between master and
> > > > the releases there seem to be many that did not move  
> > > 
> > > Can I send a patch that makes them "real" public fields, and which
> > > deprecates the accessors? In all libs?  
> > 
> > If someone first checks that they are in the same offset in their
> > structs in all FFmpeg releases with the same soname.
> > And someone checks that we have no "insert new ... field above/below
> > this line" things above them
> > 
> > Yes, definitly
> 
> The offsets wouldn't have to be the same, since they were allowed to be
> accessed by accessor only.

thats true but someone writing code to access it would likely use
the latest documentation and if that doesnt mention that earlier
versions needed a different access then likely not realize and as
that app would build and work fine against a release he wouldnt notice
until that app built against a older release is then linked to a
newer

[...]
wm4 Dec. 12, 2016, 7:28 p.m.
On Mon, 12 Dec 2016 19:55:59 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Dec 12, 2016 at 05:59:32PM +0100, wm4 wrote:
> > On Mon, 12 Dec 2016 17:34:15 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > On Mon, Dec 12, 2016 at 09:48:01AM +0100, wm4 wrote:  
> > > > On Mon, 12 Dec 2016 03:34:27 +0100
> > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > >     
> > > > > On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:    
> > > > > > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:      
> > > > > > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:      
> > > > > > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:      
> > > > > > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:      
> > > > > > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:      
> > > > > > > [...]
> > > > > > >       
> > > > > > >>> And also theres more work for us to maintain seperate implementations
> > > > > > >>> for the options, all code accessing them has to deal with them being
> > > > > > >>> in different places, making all related backports harder.
> > > > > > >>>
> > > > > > >>> To me that looks like a disadvantage from every side
> > > > > > >>>
> > > > > > >>> I think the real solution is to start liking AVOptions, they make
> > > > > > >>> our work easier.      
> > > > > > >>
> > > > > > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > > > > > >> in public structs are what's kinda ugly an unpopular.      
> > > > > > > 
> > > > > > > a slightly off topic question but
> > > > > > > if people care about these existing "no direct access" fields
> > > > > > > why do they not change them ?
> > > > > > > its a bit reading and thinking and droping the "no direct access"
> > > > > > > comments, this is not much work
> > > > > > > It requires a tiny bit of care so that future added fields dont do
> > > > > > > bad things and we should document that past releases still in some
> > > > > > > cases need the indirect access  ...
> > > > > > > 
> > > > > > > just seems a bit odd to me in relation to the opposition to add such
> > > > > > > a field were its needed for a bugfix but total apparent lack of
> > > > > > > interrest in removing such "no direct access" restrctions  where there
> > > > > > > is no reason at all to keep them      
> > > > > > 
> > > > > > They are all supposed to stop being no direct access with the next bump.
> > > > > > Afaik most are remnants of the libav ABI compatibility days, same with
> > > > > > the get/setter functions, but they can't just be removed right now because
> > > > > > there are several versions using the current ABI (3.0 and newer i think).      
> > > > > 
> > > > > theres no need to wait for the next bump with removing many of the
> > > > > "no direct access" rules because the fields always where in the same
> > > > > position in the current major version
> > > > > 
> > > > > Removing them before the bump would allow people to stop using
> > > > > the accessors earlier
> > > > > 
> > > > > someone needs to read through the struct and think about it though
> > > > > so nothing is missed but from just the diff between master and
> > > > > the releases there seem to be many that did not move    
> > > > 
> > > > Can I send a patch that makes them "real" public fields, and which
> > > > deprecates the accessors? In all libs?    
> > > 
> > > If someone first checks that they are in the same offset in their
> > > structs in all FFmpeg releases with the same soname.
> > > And someone checks that we have no "insert new ... field above/below
> > > this line" things above them
> > > 
> > > Yes, definitly  
> > 
> > The offsets wouldn't have to be the same, since they were allowed to be
> > accessed by accessor only.  
> 
> thats true but someone writing code to access it would likely use
> the latest documentation and if that doesnt mention that earlier
> versions needed a different access then likely not realize and as
> that app would build and work fine against a release he wouldnt notice
> until that app built against a older release is then linked to a
> newer

We've never cared about such cases before.
Michael Niedermayer Dec. 12, 2016, 8:02 p.m.
On Mon, Dec 12, 2016 at 08:28:40PM +0100, wm4 wrote:
> On Mon, 12 Dec 2016 19:55:59 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Mon, Dec 12, 2016 at 05:59:32PM +0100, wm4 wrote:
> > > On Mon, 12 Dec 2016 17:34:15 +0100
> > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >   
> > > > On Mon, Dec 12, 2016 at 09:48:01AM +0100, wm4 wrote:  
> > > > > On Mon, 12 Dec 2016 03:34:27 +0100
> > > > > Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > >     
> > > > > > On Sun, Dec 11, 2016 at 10:51:08PM -0300, James Almer wrote:    
> > > > > > > On 12/11/2016 10:29 PM, Michael Niedermayer wrote:      
> > > > > > > > On Sat, Dec 10, 2016 at 09:52:08PM -0300, James Almer wrote:      
> > > > > > > >> On 12/10/2016 9:23 PM, Michael Niedermayer wrote:      
> > > > > > > >>> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:      
> > > > > > > >>>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:      
> > > > > > > > [...]
> > > > > > > >       
> > > > > > > >>> And also theres more work for us to maintain seperate implementations
> > > > > > > >>> for the options, all code accessing them has to deal with them being
> > > > > > > >>> in different places, making all related backports harder.
> > > > > > > >>>
> > > > > > > >>> To me that looks like a disadvantage from every side
> > > > > > > >>>
> > > > > > > >>> I think the real solution is to start liking AVOptions, they make
> > > > > > > >>> our work easier.      
> > > > > > > >>
> > > > > > > >> AVOptions are fine. Private-but-not-really and no-direct-access fields
> > > > > > > >> in public structs are what's kinda ugly an unpopular.      
> > > > > > > > 
> > > > > > > > a slightly off topic question but
> > > > > > > > if people care about these existing "no direct access" fields
> > > > > > > > why do they not change them ?
> > > > > > > > its a bit reading and thinking and droping the "no direct access"
> > > > > > > > comments, this is not much work
> > > > > > > > It requires a tiny bit of care so that future added fields dont do
> > > > > > > > bad things and we should document that past releases still in some
> > > > > > > > cases need the indirect access  ...
> > > > > > > > 
> > > > > > > > just seems a bit odd to me in relation to the opposition to add such
> > > > > > > > a field were its needed for a bugfix but total apparent lack of
> > > > > > > > interrest in removing such "no direct access" restrctions  where there
> > > > > > > > is no reason at all to keep them      
> > > > > > > 
> > > > > > > They are all supposed to stop being no direct access with the next bump.
> > > > > > > Afaik most are remnants of the libav ABI compatibility days, same with
> > > > > > > the get/setter functions, but they can't just be removed right now because
> > > > > > > there are several versions using the current ABI (3.0 and newer i think).      
> > > > > > 
> > > > > > theres no need to wait for the next bump with removing many of the
> > > > > > "no direct access" rules because the fields always where in the same
> > > > > > position in the current major version
> > > > > > 
> > > > > > Removing them before the bump would allow people to stop using
> > > > > > the accessors earlier
> > > > > > 
> > > > > > someone needs to read through the struct and think about it though
> > > > > > so nothing is missed but from just the diff between master and
> > > > > > the releases there seem to be many that did not move    
> > > > > 
> > > > > Can I send a patch that makes them "real" public fields, and which
> > > > > deprecates the accessors? In all libs?    
> > > > 
> > > > If someone first checks that they are in the same offset in their
> > > > structs in all FFmpeg releases with the same soname.
> > > > And someone checks that we have no "insert new ... field above/below
> > > > this line" things above them
> > > > 
> > > > Yes, definitly  
> > > 
> > > The offsets wouldn't have to be the same, since they were allowed to be
> > > accessed by accessor only.  
> > 
> > thats true but someone writing code to access it would likely use
> > the latest documentation and if that doesnt mention that earlier
> > versions needed a different access then likely not realize and as
> > that app would build and work fine against a release he wouldnt notice
> > until that app built against a older release is then linked to a
> > newer
> 
> We've never cared about such cases before.

I never knowingly ignored such case before
quite possibly i did so without conciously realizing. 

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 02234aee67..8123092ac0 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3573,8 +3573,8 @@  typedef struct AVCodecContext {
     /**
      * The number of pixels per image to maximally accept.
      *
-     * - decoding: set by user
-     * - encoding: set by user
+     * - decoding: set by user through AVOptions (NO direct access)
+     * - encoding: set by user through AVOptions (NO direct access)
      */
     int64_t max_pixels;