| Message ID | 20170720204622.27337-1-atomnuker@gmail.com |
|---|---|
| State | New |
| Headers | show |
Le duodi 2 thermidor, an CCXXV, Rostislav Pehlivanov a écrit : > Many image formats support embedding of ICC profiles directly in > their bitstreams. Add a new side data type to allow exposing them to > API users. Why not make it a member of AVFrame directly? It looks to me very similar in principle to color_range, color_primaries, colorspace, etc. > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > --- > libavutil/frame.h | 6 ++++++ > libavutil/version.h | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 26261d7e40..ee899d844d 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -134,6 +134,12 @@ enum AVFrameSideDataType { > * the form of the AVContentLightMetadata struct. > */ > AV_FRAME_DATA_CONTENT_LIGHT_LEVEL, > + > + /** > + * The data contains an ICC profile with an optional name defined in the > + * metadata entry. Not being a specialist of ICC profiles, I have no idea what that means in practice. What data structure is it? > + */ > + AV_FRAME_DATA_ICC_PROFILE, > }; > > enum AVActiveFormatDescription { > diff --git a/libavutil/version.h b/libavutil/version.h > index d4f9335a2f..35987e7b50 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -80,7 +80,7 @@ > > > #define LIBAVUTIL_VERSION_MAJOR 55 > -#define LIBAVUTIL_VERSION_MINOR 68 > +#define LIBAVUTIL_VERSION_MINOR 69 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ Regards,
On 21 July 2017 at 00:08, Nicolas George <george@nsup.org> wrote: > Le duodi 2 thermidor, an CCXXV, Rostislav Pehlivanov a écrit : > > Many image formats support embedding of ICC profiles directly in > > their bitstreams. Add a new side data type to allow exposing them to > > API users. > > Why not make it a member of AVFrame directly? It looks to me very > similar in principle to color_range, color_primaries, colorspace, etc. > > It can be quite big. In some insane cases it can be bigger than the actual packet or even the uncompressed frame. Its also not strictly necessary to display something more or less looking okay, but its necessary to display something correctly. I think its better treated like we currently treat HDR by defining a side data type and letting the API users cache and use it, which is what this patch does. Side data is also refcounted so it even solves the size issue. > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > > --- > > libavutil/frame.h | 6 ++++++ > > libavutil/version.h | 2 +- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index 26261d7e40..ee899d844d 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -134,6 +134,12 @@ enum AVFrameSideDataType { > > * the form of the AVContentLightMetadata struct. > > */ > > AV_FRAME_DATA_CONTENT_LIGHT_LEVEL, > > + > > + /** > > > + * The data contains an ICC profile with an optional name defined > in the > > + * metadata entry. > > Not being a specialist of ICC profiles, I have no idea what that means > in practice. What data structure is it? > > There's a 300 page document describing the bitstream and how its used. The smallest implementation, lcms is still a few tens of thousands of lines.
Le tridi 3 thermidor, an CCXXV, Rostislav Pehlivanov a écrit : > It can be quite big. In some insane cases it can be bigger than the actual > packet or even the uncompressed frame. Its also not strictly necessary to > display something more or less looking okay, but its necessary to display > something correctly. I think its better treated like we currently treat HDR > by defining a side data type and letting the API users cache and use it, > which is what this patch does. All this could apply to a dedicated field. Using side data only brings a type pruning of the actual type. Except: > Side data is also refcounted so it even > solves the size issue. Indeed. A separate type could be refcounted too, though, but that would require a little more code. Short-term convenience vs. long-term convenience. > > > + * The data contains an ICC profile with an optional name defined > > in the > > > + * metadata entry. > > Not being a specialist of ICC profiles, I have no idea what that means > > in practice. What data structure is it? > There's a 300 page document describing the bitstream and how its used. The > smallest implementation, lcms is still a few tens of thousands of lines. This is not what I am asking. Look at the doxy for AV_FRAME_DATA_SPHERICAL. Explaining the semantic of the structure requires at least a few pages of documentation, but the doxy still explains in a few world the data structure used. What I am asking is very simple: if I want to exploit this side through a library, to what type shall I cast the pointer? Regards,
On 21 July 2017 at 00:49, Nicolas George <george@nsup.org> wrote: > Le tridi 3 thermidor, an CCXXV, Rostislav Pehlivanov a écrit : > > It can be quite big. In some insane cases it can be bigger than the > actual > > packet or even the uncompressed frame. Its also not strictly necessary to > > display something more or less looking okay, but its necessary to display > > something correctly. I think its better treated like we currently treat > HDR > > by defining a side data type and letting the API users cache and use it, > > which is what this patch does. > > All this could apply to a dedicated field. Using side data only brings a > type pruning of the actual type. Except: > > Yes, it could. However I still think having it as a side data is better since its the easiest way and that's what all API users currently use to retrieve HDR metadata as well. > > Side data is also refcounted so it even > > solves the size issue. > > Indeed. A separate type could be refcounted too, though, but that would > require a little more code. Short-term convenience vs. long-term > convenience. > > > > > + * The data contains an ICC profile with an optional name > defined > > > in the > > > > + * metadata entry. > > > Not being a specialist of ICC profiles, I have no idea what that means > > > in practice. What data structure is it? > > There's a 300 page document describing the bitstream and how its used. > The > > smallest implementation, lcms is still a few tens of thousands of lines. > > This is not what I am asking. Look at the doxy for > AV_FRAME_DATA_SPHERICAL. Explaining the semantic of the structure > requires at least a few pages of documentation, but the doxy still > explains in a few world the data structure used. > > What I am asking is very simple: if I want to exploit this side through > a library, to what type shall I cast the pointer? > > Nothing, its a bitstream. You give it to a library to decode and the library is then ready to take e.g. rgb and output color corrected rgb.
On 7/21/2017 12:55 AM, Rostislav Pehlivanov wrote: >> All this could apply to a dedicated field. Using side data only brings a >> type pruning of the actual type. Except: >> >> > Yes, it could. However I still think having it as a side data is better > since its the easiest way and that's what all API users currently use to > retrieve HDR metadata as well. +1 on exposing it as side data; it's consistent with previous APIs and doesn't require Yet Another Field for something only a few formats (PNG, JPG, TIFF, MP4) can contain. >> This is not what I am asking. Look at the doxy for >> AV_FRAME_DATA_SPHERICAL. Explaining the semantic of the structure >> requires at least a few pages of documentation, but the doxy still >> explains in a few world the data structure used. >> >> What I am asking is very simple: if I want to exploit this side through >> a library, to what type shall I cast the pointer? >> >> > Nothing, its a bitstream. You give it to a library to decode and the > library is then ready to take e.g. rgb and output color corrected rgb. FWIW, literally every other library exposes ICC data the same way (as a dumb buffer), and it's IMO the most reasonable. It's not reasonable to put an actual description of ICC data in the doxy (see: 300 page spec... it is complictated). Just a reference to the spec is fine. I say this as a downstream user of various libraries having had to use ICC profiles (always with lcms2, of course). - Derek
Le tridi 3 thermidor, an CCXXV, Derek Buitenhuis a écrit : > +1 on exposing it as side data; it's consistent with previous APIs and doesn't I will not oppose further; I do not buy the "Yet Another Field" argument but I will debunk it another time. > FWIW, literally every other library exposes ICC data the same way (as a dumb > buffer), and it's IMO the most reasonable. It's not reasonable to put an actual > description of ICC data in the doxy (see: 300 page spec... it is complictated). > Just a reference to the spec is fine. I fully agree. I suggest something along the lines: The data contains an ICC profile as an opaque octet buffer following the format described at $url with an optional name defined in the metadata entry "name". Regards,
On 21 July 2017 at 14:36, Nicolas George <george@nsup.org> wrote: > Le tridi 3 thermidor, an CCXXV, Derek Buitenhuis a écrit : > > +1 on exposing it as side data; it's consistent with previous APIs and > doesn't > > I will not oppose further; I do not buy the "Yet Another Field" argument > but I will debunk it another time. > > > FWIW, literally every other library exposes ICC data the same way (as a > dumb > > buffer), and it's IMO the most reasonable. It's not reasonable to put an > actual > > description of ICC data in the doxy (see: 300 page spec... it is > complictated). > > Just a reference to the spec is fine. > > I fully agree. I suggest something along the lines: > > The data contains an ICC profile as an opaque octet buffer > following the format described at $url with an optional name > defined in the metadata entry "name". > > Regards, > Thanks for the suggestion, changed the description to what you suggested. I'll push both patches tomorrow unless someone finds something else wrong.
On 22 July 2017 at 21:17, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > > > On 21 July 2017 at 14:36, Nicolas George <george@nsup.org> wrote: > >> Le tridi 3 thermidor, an CCXXV, Derek Buitenhuis a écrit : >> > +1 on exposing it as side data; it's consistent with previous APIs and >> doesn't >> >> I will not oppose further; I do not buy the "Yet Another Field" argument >> but I will debunk it another time. >> >> > FWIW, literally every other library exposes ICC data the same way (as a >> dumb >> > buffer), and it's IMO the most reasonable. It's not reasonable to put >> an actual >> > description of ICC data in the doxy (see: 300 page spec... it is >> complictated). >> > Just a reference to the spec is fine. >> >> I fully agree. I suggest something along the lines: >> >> The data contains an ICC profile as an opaque octet buffer >> following the format described at $url with an optional name >> defined in the metadata entry "name". >> >> Regards, >> > > Thanks for the suggestion, changed the description to what you suggested. > > I'll push both patches tomorrow unless someone finds something else wrong. > Thanks for your reviews, pushed. Replaced $url with the ISO spec the format follows.
diff --git a/libavutil/frame.h b/libavutil/frame.h index 26261d7e40..ee899d844d 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -134,6 +134,12 @@ enum AVFrameSideDataType { * the form of the AVContentLightMetadata struct. */ AV_FRAME_DATA_CONTENT_LIGHT_LEVEL, + + /** + * The data contains an ICC profile with an optional name defined in the + * metadata entry. + */ + AV_FRAME_DATA_ICC_PROFILE, }; enum AVActiveFormatDescription { diff --git a/libavutil/version.h b/libavutil/version.h index d4f9335a2f..35987e7b50 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -80,7 +80,7 @@ #define LIBAVUTIL_VERSION_MAJOR 55 -#define LIBAVUTIL_VERSION_MINOR 68 +#define LIBAVUTIL_VERSION_MINOR 69 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
Many image formats support embedding of ICC profiles directly in their bitstreams. Add a new side data type to allow exposing them to API users. Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> --- libavutil/frame.h | 6 ++++++ libavutil/version.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)