| Message ID | 20240925225219.3060-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avformat/avformat: add a new disposition to signal the stream is an HDR gainmap | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 9/26/2024 3:00 AM, Anton Khirnov wrote: > Quoting James Almer (2024-09-26 00:52:16) >> HDR images photos taken by certain cameras split this as a separate image. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/avformat.h | 5 +++++ >> libavformat/dump.c | 2 ++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >> index 56c1c80289..6d9f5c4399 100644 >> --- a/libavformat/avformat.h >> +++ b/libavformat/avformat.h >> @@ -718,6 +718,11 @@ typedef struct AVIndexEntry { >> * Annex G/H, or HEVC Annex F). >> */ >> #define AV_DISPOSITION_MULTILAYER (1 << 21) >> +/** >> + * The video stream contains an HDR gainmap. Only ever used with >> + * AV_DISPOSITION_DEPENDENT. >> + */ >> +#define AV_DISPOSITION_GAINMAP (1 << 22) > > Presumably we want this information available in codecs and filters as > well, so then should it not be side data instead? There is no other information than "This is a gainmap", and that's container level information (Same as "This is a tile" for heif). How would side data work for this? I'm including it in the tile grid stream group in patch 4/4, so that should give the caller all the information they need.
Quoting James Almer (2024-09-26 14:06:25) > On 9/26/2024 3:00 AM, Anton Khirnov wrote: > > Quoting James Almer (2024-09-26 00:52:16) > >> HDR images photos taken by certain cameras split this as a separate image. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavformat/avformat.h | 5 +++++ > >> libavformat/dump.c | 2 ++ > >> 2 files changed, 7 insertions(+) > >> > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >> index 56c1c80289..6d9f5c4399 100644 > >> --- a/libavformat/avformat.h > >> +++ b/libavformat/avformat.h > >> @@ -718,6 +718,11 @@ typedef struct AVIndexEntry { > >> * Annex G/H, or HEVC Annex F). > >> */ > >> #define AV_DISPOSITION_MULTILAYER (1 << 21) > >> +/** > >> + * The video stream contains an HDR gainmap. Only ever used with > >> + * AV_DISPOSITION_DEPENDENT. > >> + */ > >> +#define AV_DISPOSITION_GAINMAP (1 << 22) > > > > Presumably we want this information available in codecs and filters as > > well, so then should it not be side data instead? > > There is no other information than "This is a gainmap", and that's > container level information (Same as "This is a tile" for heif). How > would side data work for this? You could e.g. have side data that has no payload, and its presence signals the same information. > I'm including it in the tile grid stream group in patch 4/4, so that > should give the caller all the information they need. Assuming all they ever need is handle everything manually.
On 9/26/2024 11:17 AM, Anton Khirnov wrote: > Quoting James Almer (2024-09-26 14:06:25) >> On 9/26/2024 3:00 AM, Anton Khirnov wrote: >>> Quoting James Almer (2024-09-26 00:52:16) >>>> HDR images photos taken by certain cameras split this as a separate image. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavformat/avformat.h | 5 +++++ >>>> libavformat/dump.c | 2 ++ >>>> 2 files changed, 7 insertions(+) >>>> >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>> index 56c1c80289..6d9f5c4399 100644 >>>> --- a/libavformat/avformat.h >>>> +++ b/libavformat/avformat.h >>>> @@ -718,6 +718,11 @@ typedef struct AVIndexEntry { >>>> * Annex G/H, or HEVC Annex F). >>>> */ >>>> #define AV_DISPOSITION_MULTILAYER (1 << 21) >>>> +/** >>>> + * The video stream contains an HDR gainmap. Only ever used with >>>> + * AV_DISPOSITION_DEPENDENT. >>>> + */ >>>> +#define AV_DISPOSITION_GAINMAP (1 << 22) >>> >>> Presumably we want this information available in codecs and filters as >>> well, so then should it not be side data instead? >> >> There is no other information than "This is a gainmap", and that's >> container level information (Same as "This is a tile" for heif). How >> would side data work for this? > > You could e.g. have side data that has no payload, and its presence > signals the same information. Is there precedent for this? Would it be a side data with size 0 (but data still pointing to an AVBufferRef)? > >> I'm including it in the tile grid stream group in patch 4/4, so that >> should give the caller all the information they need. > > Assuming all they ever need is handle everything manually. I don't understand what you mean. A side data entry, assuming it's propagated to the decoder context and then the decoded frame, would indeed pass this information through but will still be missing what image this gainmap is for, so the caller still needs to be aware of it (This is what the group is for) and do some things manually, namely inserting an hypothetical filter for the merging process.
Quoting James Almer (2024-09-26 16:25:11) > On 9/26/2024 11:17 AM, Anton Khirnov wrote: > > Quoting James Almer (2024-09-26 14:06:25) > >> On 9/26/2024 3:00 AM, Anton Khirnov wrote: > >>> Quoting James Almer (2024-09-26 00:52:16) > >>>> HDR images photos taken by certain cameras split this as a separate image. > >>>> > >>>> Signed-off-by: James Almer <jamrial@gmail.com> > >>>> --- > >>>> libavformat/avformat.h | 5 +++++ > >>>> libavformat/dump.c | 2 ++ > >>>> 2 files changed, 7 insertions(+) > >>>> > >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >>>> index 56c1c80289..6d9f5c4399 100644 > >>>> --- a/libavformat/avformat.h > >>>> +++ b/libavformat/avformat.h > >>>> @@ -718,6 +718,11 @@ typedef struct AVIndexEntry { > >>>> * Annex G/H, or HEVC Annex F). > >>>> */ > >>>> #define AV_DISPOSITION_MULTILAYER (1 << 21) > >>>> +/** > >>>> + * The video stream contains an HDR gainmap. Only ever used with > >>>> + * AV_DISPOSITION_DEPENDENT. > >>>> + */ > >>>> +#define AV_DISPOSITION_GAINMAP (1 << 22) > >>> > >>> Presumably we want this information available in codecs and filters as > >>> well, so then should it not be side data instead? > >> > >> There is no other information than "This is a gainmap", and that's > >> container level information (Same as "This is a tile" for heif). How > >> would side data work for this? > > > > You could e.g. have side data that has no payload, and its presence > > signals the same information. > > Is there precedent for this? Not as far as I know. But I also don't see why couldn't it be done. > Would it be a side data with size 0 (but data still pointing to an > AVBufferRef)? Packet side data is not refcounted. Otherwise yes, I imagine it would have 0 size, unless we figure out some useful content. > > > >> I'm including it in the tile grid stream group in patch 4/4, so that > >> should give the caller all the information they need. > > > > Assuming all they ever need is handle everything manually. > > I don't understand what you mean. What I mean is - if we ever want to propagate this information to decoders/filters, we probably want it to be side data. Then the disposition would be redudundant. And since we are not far from running out of disposition space, we might as well make it side data only. It could also conceivably be a frame/packet flag.
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 56c1c80289..6d9f5c4399 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -718,6 +718,11 @@ typedef struct AVIndexEntry { * Annex G/H, or HEVC Annex F). */ #define AV_DISPOSITION_MULTILAYER (1 << 21) +/** + * The video stream contains an HDR gainmap. Only ever used with + * AV_DISPOSITION_DEPENDENT. + */ +#define AV_DISPOSITION_GAINMAP (1 << 22) /** * @return The AV_DISPOSITION_* flag corresponding to disp or a negative error diff --git a/libavformat/dump.c b/libavformat/dump.c index f20c2c4953..5178f19685 100644 --- a/libavformat/dump.c +++ b/libavformat/dump.c @@ -589,6 +589,8 @@ static void dump_disposition(int disposition, int log_level) av_log(NULL, log_level, " (non-diegetic)"); if (disposition & AV_DISPOSITION_MULTILAYER) av_log(NULL, log_level, " (multilayer)"); + if (disposition & AV_DISPOSITION_GAINMAP) + av_log(NULL, log_level, " (hdr gainmap)"); } /* "user interface" functions */
HDR images photos taken by certain cameras split this as a separate image. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/avformat.h | 5 +++++ libavformat/dump.c | 2 ++ 2 files changed, 7 insertions(+)