| Message ID | 20171106173839.3648-1-atomnuker@gmail.com |
|---|---|
| State | New |
| Headers | show |
Le sextidi 16 brumaire, an CCXXVI, Rostislav Pehlivanov a écrit : > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > --- > doc/APIchanges | 3 +++ > libavutil/frame.h | 6 ++++++ > libavutil/version.h | 2 +- > 3 files changed, 10 insertions(+), 1 deletion(-) Sorry if it has come up before, but why not just add AVRational gamma; with 0/0 meaning unspecified? It seems like a relevant information, at least as much as AVColor*. And overall much simpler. Regards,
On 6 November 2017 at 18:03, Nicolas George <george@nsup.org> wrote: > Le sextidi 16 brumaire, an CCXXVI, Rostislav Pehlivanov a écrit : > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > > --- > > doc/APIchanges | 3 +++ > > libavutil/frame.h | 6 ++++++ > > libavutil/version.h | 2 +- > > 3 files changed, 10 insertions(+), 1 deletion(-) > > Sorry if it has come up before, but why not just add > > AVRational gamma; > > with 0/0 meaning unspecified? It seems like a relevant information, at > least as much as AVColor*. And overall much simpler. > > Regards, > > -- > Nicolas George > Gamma info is related to mastering info so API users expect to get both using the same API rather than look for new fields in avframe and evaluate whether they're different on a per-frame basis. Also, it prevents from adding more fields to avframe and making a bigger mess.
On Tue, Nov 07, 2017 at 08:13:57PM +0000, Rostislav Pehlivanov wrote: > On 6 November 2017 at 18:03, Nicolas George <george@nsup.org> wrote: > > > Le sextidi 16 brumaire, an CCXXVI, Rostislav Pehlivanov a écrit : > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > > > --- > > > doc/APIchanges | 3 +++ > > > libavutil/frame.h | 6 ++++++ > > > libavutil/version.h | 2 +- > > > 3 files changed, 10 insertions(+), 1 deletion(-) > > > > Sorry if it has come up before, but why not just add > > > > AVRational gamma; > > > > with 0/0 meaning unspecified? It seems like a relevant information, at > > least as much as AVColor*. And overall much simpler. > > > > Regards, > > > > -- > > Nicolas George > > > > Gamma info is related to mastering info so API users expect to get both > using the same API rather than look for new fields in avframe and evaluate > whether they're different on a per-frame basis. > Also, it prevents from adding more fields to avframe and making a bigger > mess. If a filter changes gamma, it will then have to update the side data if its in side data I think i understand that you may see gamma more as a input device characteristic. But really its part of the mapping of the physical worlds light spectrum shape for each pixel to the 3 element vector used to represent pixels. As such if the representation of pixels change, gamma may change Take a very simple example, simple rescaling or similar filtering, all these filters should be performed with a flat 1.0 gamma to be physically correct. So often a whole filter chain would be more correct by converting the gamma != 1.0 8bit per sample to a gamma = 1.0 bps > 8bit first and at the end of the chain convert back to a more commonly used gamma != 1.0. We dont do this but we should support it at least optionally and having gamma only in side data would make it a bit unwieldy [...]
On Wed, Nov 08, 2017 at 11:50:34AM +0100, Michael Niedermayer wrote: > On Tue, Nov 07, 2017 at 08:13:57PM +0000, Rostislav Pehlivanov wrote: > > On 6 November 2017 at 18:03, Nicolas George <george@nsup.org> wrote: > > > > > Le sextidi 16 brumaire, an CCXXVI, Rostislav Pehlivanov a écrit : > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > > > > --- > > > > doc/APIchanges | 3 +++ > > > > libavutil/frame.h | 6 ++++++ > > > > libavutil/version.h | 2 +- > > > > 3 files changed, 10 insertions(+), 1 deletion(-) > > > > > > Sorry if it has come up before, but why not just add > > > > > > AVRational gamma; > > > > > > with 0/0 meaning unspecified? It seems like a relevant information, at > > > least as much as AVColor*. And overall much simpler. > > > > > > Regards, > > > > > > -- > > > Nicolas George > > > > > > > Gamma info is related to mastering info so API users expect to get both > > using the same API rather than look for new fields in avframe and evaluate > > whether they're different on a per-frame basis. > > Also, it prevents from adding more fields to avframe and making a bigger > > mess. > > If a filter changes gamma, it will then have to update the side data > if its in side data > > I think i understand that you may see gamma more as a input device > characteristic. But really its part of the mapping of the physical > worlds light spectrum shape for each pixel to the 3 element vector > used to represent pixels. > > As such if the representation of pixels change, gamma may change > > Take a very simple example, simple rescaling or similar filtering, > all these filters should be performed with a flat 1.0 gamma to be > physically correct. So often a whole filter chain would be more > correct by converting the gamma != 1.0 8bit per sample to a gamma = 1.0 > bps > 8bit first and at the end of the chain convert back to a more > commonly used gamma != 1.0. > We dont do this but we should support it at least optionally and > having gamma only in side data would make it a bit unwieldy we also could have a set of recoding device parameters in side data (mastering info) and the current frames data[] gamma in avframe. not sure thats a good idea or too confusing but it would allow a filter to convert back to the input devices gamma without its value being given to the filter as its in side data still [...]
On 11/8/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Nov 08, 2017 at 11:50:34AM +0100, Michael Niedermayer wrote: >> On Tue, Nov 07, 2017 at 08:13:57PM +0000, Rostislav Pehlivanov wrote: >> > On 6 November 2017 at 18:03, Nicolas George <george@nsup.org> wrote: >> > >> > > Le sextidi 16 brumaire, an CCXXVI, Rostislav Pehlivanov a ecrit : >> > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> >> > > > --- >> > > > doc/APIchanges | 3 +++ >> > > > libavutil/frame.h | 6 ++++++ >> > > > libavutil/version.h | 2 +- >> > > > 3 files changed, 10 insertions(+), 1 deletion(-) >> > > >> > > Sorry if it has come up before, but why not just add >> > > >> > > AVRational gamma; >> > > >> > > with 0/0 meaning unspecified? It seems like a relevant information, >> > > at >> > > least as much as AVColor*. And overall much simpler. >> > > >> > > Regards, >> > > >> > > -- >> > > Nicolas George >> > > >> > >> > Gamma info is related to mastering info so API users expect to get both >> > using the same API rather than look for new fields in avframe and >> > evaluate >> > whether they're different on a per-frame basis. >> > Also, it prevents from adding more fields to avframe and making a >> > bigger >> > mess. >> >> If a filter changes gamma, it will then have to update the side data >> if its in side data >> >> I think i understand that you may see gamma more as a input device >> characteristic. But really its part of the mapping of the physical >> worlds light spectrum shape for each pixel to the 3 element vector >> used to represent pixels. >> >> As such if the representation of pixels change, gamma may change >> >> Take a very simple example, simple rescaling or similar filtering, >> all these filters should be performed with a flat 1.0 gamma to be >> physically correct. So often a whole filter chain would be more >> correct by converting the gamma != 1.0 8bit per sample to a gamma = 1.0 >> bps > 8bit first and at the end of the chain convert back to a more >> commonly used gamma != 1.0. >> We dont do this but we should support it at least optionally and >> having gamma only in side data would make it a bit unwieldy > > we also could have a set of recoding device parameters in side data > (mastering info) > > and the current frames data[] gamma in avframe. > not sure thats a good idea or too confusing but it would allow > a filter to convert back to the input devices gamma without its value > being given to the filter as its in side data still No, no and no. Gamma is not always available and polluting AVFrame is big no. I vote and veto against your and Nicolas idea.
L'octidi 18 brumaire, an CCXXVI, Paul B Mahol a écrit : > No, no and no. Gamma is not always available and polluting AVFrame is big no. The same arguments would apply for pict_type, channel_layout, color_*, crop_*. This is no pollution, it is a minor but relevant information about the frame. > I vote and veto against your and Nicolas idea. What a nice way of conducing a discussion. Consider I vote and veto against adding side data. Regards,
On 11/8/17, Nicolas George <george@nsup.org> wrote: > L'octidi 18 brumaire, an CCXXVI, Paul B Mahol a ecrit : >> No, no and no. Gamma is not always available and polluting AVFrame is big >> no. > > The same arguments would apply for pict_type, channel_layout, color_*, > crop_*. This is no pollution, it is a minor but relevant information > about the frame. Why are you doing this? Your bikesheds do not apply anymore. You just like to make your opinions important to block other devs hard work. What's next? Adding more entries like this? Only PNG appears to set gamma and others simply do not care.
L'octidi 18 brumaire, an CCXXVI, Paul B Mahol a écrit : > Why are you doing this? Your bikesheds do not apply anymore. I am "doing this" because I care about the quality and design of the API, no more. In this particular instance, the side data API is now completely braindead, a huge waste in terms of code complexity for no actual benefit. It needs to be phased out, not expanded. > You just like to make your opinions important to block other devs hard work. I am not the one who introduced a veto in this discussion, you did. Withdraw it and we will be able to discuss with arguments. Regards,
On 11/8/17, Nicolas George <george@nsup.org> wrote: > L'octidi 18 brumaire, an CCXXVI, Paul B Mahol a ecrit : >> Why are you doing this? Your bikesheds do not apply anymore. > > I am "doing this" because I care about the quality and design of the > API, no more. In this particular instance, the side data API is now > completely braindead, a huge waste in terms of code complexity for no > actual benefit. It needs to be phased out, not expanded. If you want one big 2mb size AVFrame go ahead and fork FFmpeg.
On Wed, Nov 8, 2017 at 8:31 PM, Nicolas George <george@nsup.org> wrote: > L'octidi 18 brumaire, an CCXXVI, Paul B Mahol a écrit : >> Why are you doing this? Your bikesheds do not apply anymore. > > I am "doing this" because I care about the quality and design of the > API, no more. In this particular instance, the side data API is now > completely braindead, a huge waste in terms of code complexity for no > actual benefit. It needs to be phased out, not expanded. > If you want to discuss and possibly replace the frame sidedata API, then start such a discussion independent of a small feature patch. In the meantime, it does no real harm to add one more sidedata type to the numerous list of types we already have, and lets people carry on with their work. - Hendrik
L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a écrit : > In the meantime, it does no real harm to add one more sidedata type to > the numerous list of types we already have, and lets people carry on > with their work. It does harm, look at the other patch: 5 lines of useless clutter out of 10 lines for the feature, using side data for that features has a 100% overhead. And that is not only for the code here, it has the same consequences for the applications that will use that API as well: they have to do the same errors checks, they have to handle a type-pruned pointer, check the size of the data, etc. This is a terrible API, there is no doubt that people would prefer a single field. By all means let us discuss all this. But if Paul maintains his veto, I maintain mine. Please persuade Paul to behave like an adult of make good on his trice-repeated promise of forking. Regards,
On Wed, Nov 8, 2017 at 9:36 PM, Nicolas George <george@nsup.org> wrote: > This is a terrible API, there is no doubt that > people would prefer a single field. > I don't. We've done a lot of work to remove fields from generic data structures which are used by single codecs on semi-obscure cases. The fact alone that after years of avcodec and a PNG decoder this field wasn't asked for regularly alone speaks to me that its not generic enough to warrant inclusion in AVFrame. Certainly it would make for nicer code if you could access every field directly, but if we do that for every piece of data provided by all random single decoders, we have a nightmare of a frame structure, with hundreds of fields that only ever are applicable in a minority of cases. - Hendrik
On 11/8/17, Nicolas George <george@nsup.org> wrote: > L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a ecrit : >> In the meantime, it does no real harm to add one more sidedata type to >> the numerous list of types we already have, and lets people carry on >> with their work. > > It does harm, look at the other patch: 5 lines of useless clutter out of > 10 lines for the feature, using side data for that features has a 100% > overhead. > > And that is not only for the code here, it has the same consequences for > the applications that will use that API as well: they have to do the > same errors checks, they have to handle a type-pruned pointer, check the > size of the data, etc. This is a terrible API, there is no doubt that > people would prefer a single field. > > By all means let us discuss all this. But if Paul maintains his veto, I > maintain mine. Please persuade Paul to behave like an adult of make good > on his trice-repeated promise of forking. Maybe I promised a fork, but looks like people are already forking FFmpeg because of people like you, latest one being ffmpeg-mpv. There should be new call for voting commitee to drop you, Michael and Carl from veto voting commitee. You guys should not block others work.
On 11/8/2017 5:52 PM, Paul B Mahol wrote: > On 11/8/17, Nicolas George <george@nsup.org> wrote: >> L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a ecrit : >>> In the meantime, it does no real harm to add one more sidedata type to >>> the numerous list of types we already have, and lets people carry on >>> with their work. >> >> It does harm, look at the other patch: 5 lines of useless clutter out of >> 10 lines for the feature, using side data for that features has a 100% >> overhead. >> >> And that is not only for the code here, it has the same consequences for >> the applications that will use that API as well: they have to do the >> same errors checks, they have to handle a type-pruned pointer, check the >> size of the data, etc. This is a terrible API, there is no doubt that >> people would prefer a single field. >> >> By all means let us discuss all this. But if Paul maintains his veto, I >> maintain mine. Please persuade Paul to behave like an adult of make good >> on his trice-repeated promise of forking. > > Maybe I promised a fork, but looks like people are already forking FFmpeg > because of people like you, latest one being ffmpeg-mpv. > > There should be new call for voting commitee to drop you, Michael and Carl > from veto voting commitee. You guys should not block others work. Please, everyone, it's a PNG patch. Why are any of you even remotely ok with turning it into a project flamewar?
On 8 November 2017 at 20:52, Paul B Mahol <onemda@gmail.com> wrote: > On 11/8/17, Nicolas George <george@nsup.org> wrote: > > L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a ecrit : > >> In the meantime, it does no real harm to add one more sidedata type to > >> the numerous list of types we already have, and lets people carry on > >> with their work. > > > > It does harm, look at the other patch: 5 lines of useless clutter out of > > 10 lines for the feature, using side data for that features has a 100% > > overhead. > > > > And that is not only for the code here, it has the same consequences for > > the applications that will use that API as well: they have to do the > > same errors checks, they have to handle a type-pruned pointer, check the > > size of the data, etc. This is a terrible API, there is no doubt that > > people would prefer a single field. > > > > By all means let us discuss all this. But if Paul maintains his veto, I > > maintain mine. Please persuade Paul to behave like an adult of make good > > on his trice-repeated promise of forking. > > Maybe I promised a fork, but looks like people are already forking FFmpeg > because of people like you, latest one being ffmpeg-mpv. > > There should be new call for voting commitee to drop you, Michael and Carl > from veto voting commitee. You guys should not block others work. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Chill, different opinions important. I don't particularly care about the patch really, its only exposed by png and never used outside of joke images.
L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a écrit : > I don't. We've done a lot of work to remove fields from generic data > structures which are used by single codecs on semi-obscure cases. The > fact alone that after years of avcodec and a PNG decoder this field > wasn't asked for regularly alone speaks to me that its not generic > enough to warrant inclusion in AVFrame. > > Certainly it would make for nicer code if you could access every field > directly, but if we do that for every piece of data provided by all > random single decoders, we have a nightmare of a frame structure, with > hundreds of fields that only ever are applicable in a minority of > cases. This is a "slippery slope" kind of argument, and they are rarely valid. Right now, we have only 16 frame side data types, and the conditions to accept them were less stringent that they would be for fields in the frame. Your "hundreds of fields" is quite an exaggeration. The question we should be asking is: does it make sense, in general, for any frame (or any video frame, or...)? For this field, I would argue that the answer is yes: the gamma value is needed to make a few operations on frames really correct. You can for example observe that libswscale can do gamma-correct scaling, but it hardcodes 2.2. Furthermore, I am quite doubtful about another side of the argument: you complain about "hundreds of fields" that make the AVFrame structure a "nightmare", but please explain to me how it is worse a nightmare than having "hundreds of" side data types defined in the big enum just a few lines above AVFrame. The only difference I can see is the size of the structure. I grant you that for really hundreds of fields it would indeed be a concern. But not for sixteen. Apart from that, the problem is about the number of features that a developers needs to know, and a side data type counts for that exactly as much as a field in AVFrame. Regards,
On Wed, Nov 8, 2017 at 10:30 PM, Nicolas George <george@nsup.org> wrote: > > The question we should be asking is: does it make sense, in general, for > any frame (or any video frame, or...)? For this field, I would argue > that the answer is yes: the gamma value is needed to make a few > operations on frames really correct. You can for example observe that > libswscale can do gamma-correct scaling, but it hardcodes 2.2. > Gamma might sound useful of a property to have, but there is no actual technical evidence to really support that. As said in an earlier mail, we've gone for years without it and no-one missed it, and even now its only being added for one decoder, and even there its apparently only used for obscure files ("joke images", as the author called it). In fact, for videos, the gamme is usually defined through the transfer function, which has the color_trc field already. > Furthermore, I am quite doubtful about another side of the argument: you > complain about "hundreds of fields" that make the AVFrame structure a > "nightmare", but please explain to me how it is worse a nightmare than > having "hundreds of" side data types defined in the big enum just a few > lines above AVFrame. The only difference I can see is the size of the > structure. I grant you that for really hundreds of fields it would > indeed be a concern. But not for sixteen. Apart from that, the problem > is about the number of features that a developers needs to know, and a > side data type counts for that exactly as much as a field in AVFrame. I actually think that the separation into "core" data and "side" data helps clarity. The AVFrame contains fields that most people should probably look at, so they can read the struct and figure out what fields may need handling. If that list includes all sorts of properties with marginal value and rare/special usage, its easy to get "lost" in there. - Hendrik
L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a écrit : > Gamma might sound useful of a property to have, but there is no actual > technical evidence to really support that. As said in an earlier mail, > we've gone for years without it and no-one missed it, and even now its > only being added for one decoder, and even there its apparently only We went for years with a channel count and no channel layout too. > used for obscure files ("joke images", as the author called it). This would be an argument to not include it at all. > I actually think that the separation into "core" data and "side" data > helps clarity. The AVFrame contains fields that most people should > probably look at, so they can read the struct and figure out what > fields may need handling. If that list includes all sorts of > properties with marginal value and rare/special usage, its easy to get > "lost" in there. The same result can be achieved with a doxy group and corresponding documentation. Note: I maintain my veto as long as Paul maintains his, as a matter of principle. But feel free to eventually push and disregard them, showing that the vetoes were worthless in the first place and that Paul's was just an intimidation tactic to close the discussion. It would make a good precedent. Regards,
On 11/8/2017 7:15 PM, Nicolas George wrote: > L'octidi 18 brumaire, an CCXXVI, Hendrik Leppkes a écrit : >> Gamma might sound useful of a property to have, but there is no actual >> technical evidence to really support that. As said in an earlier mail, >> we've gone for years without it and no-one missed it, and even now its >> only being added for one decoder, and even there its apparently only > > We went for years with a channel count and no channel layout too. > >> used for obscure files ("joke images", as the author called it). > > This would be an argument to not include it at all. Probably a good idea. Maybe just add the gamma value as a frame AVDictionary metadata entry? If the idea is that the API user can get a single value from the PNG image in some form, wouldn't side data and a dict metadata entry work the same? The latter is much cleaner and easier to remove if needed.
On 6 November 2017 at 17:38, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > --- > doc/APIchanges | 3 +++ > libavutil/frame.h | 6 ++++++ > libavutil/version.h | 2 +- > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 1490d67f27..75051deaf8 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2017-11-06 - xxxxxxx - lavu 56.01.100 - frame.h > + Add the AV_FRAME_DATA_GAMMA side data type. > + > -------- 8< --------- FFmpeg 3.4 was cut here -------- 8< --------- > > 2017-09-28 - b6cf66ae1c - lavc 57.106.104 - avcodec.h > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 0c6aab1c02..64dcf3a397 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -141,6 +141,12 @@ enum AVFrameSideDataType { > * metadata key entry "name". > */ > AV_FRAME_DATA_ICC_PROFILE, > + > + /** > + * The data contains an AVRational which describes the exponent > needed to > + * compensate for nonlinearity in the input signal. > + */ > + AV_FRAME_DATA_GAMMA, > }; > > enum AVActiveFormatDescription { > diff --git a/libavutil/version.h b/libavutil/version.h > index 1bc4b2a6cb..cf8ec498e4 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -80,7 +80,7 @@ > > > #define LIBAVUTIL_VERSION_MAJOR 56 > -#define LIBAVUTIL_VERSION_MINOR 0 > +#define LIBAVUTIL_VERSION_MINOR 1 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > -- > 2.15.0.403.gc27cc4dac6 > > Patch dropped, jamrial suggested a much more reasonable approach by using the metadata.
L'octidi 18 brumaire, an CCXXVI, James Almer a écrit : > Probably a good idea. > > Maybe just add the gamma value as a frame AVDictionary metadata entry? > If the idea is that the API user can get a single value from the PNG > image in some form, wouldn't side data and a dict metadata entry work > the same? The latter is much cleaner and easier to remove if needed. I am fine with this solution. Regards,
diff --git a/doc/APIchanges b/doc/APIchanges index 1490d67f27..75051deaf8 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2017-11-06 - xxxxxxx - lavu 56.01.100 - frame.h + Add the AV_FRAME_DATA_GAMMA side data type. + -------- 8< --------- FFmpeg 3.4 was cut here -------- 8< --------- 2017-09-28 - b6cf66ae1c - lavc 57.106.104 - avcodec.h diff --git a/libavutil/frame.h b/libavutil/frame.h index 0c6aab1c02..64dcf3a397 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -141,6 +141,12 @@ enum AVFrameSideDataType { * metadata key entry "name". */ AV_FRAME_DATA_ICC_PROFILE, + + /** + * The data contains an AVRational which describes the exponent needed to + * compensate for nonlinearity in the input signal. + */ + AV_FRAME_DATA_GAMMA, }; enum AVActiveFormatDescription { diff --git a/libavutil/version.h b/libavutil/version.h index 1bc4b2a6cb..cf8ec498e4 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -80,7 +80,7 @@ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 0 +#define LIBAVUTIL_VERSION_MINOR 1 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> --- doc/APIchanges | 3 +++ libavutil/frame.h | 6 ++++++ libavutil/version.h | 2 +- 3 files changed, 10 insertions(+), 1 deletion(-)