| Message ID | 20161210220104.14461-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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; > >
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. [...]
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 [...]
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.
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.
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 [...]
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
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,
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 [...]
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.
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) [...]
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,
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 [...]
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.
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
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?
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 [...]
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.
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 [...]
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.
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. [...]
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;
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(-)