[FFmpeg-devel] avcodec/dpxenc: support colour metadata in DPX encoder, fixes ticket #6023

Submitted by Kieran O Leary on Feb. 1, 2017, 12:42 p.m.

Details

Message ID CAO7v-1QYW6XPmbmY0gjaCuFBR8Bs5oURQ8aPoHgqoz=epd__hg@mail.gmail.com
State New
Headers show

Commit Message

Kieran O Leary Feb. 1, 2017, 12:42 p.m.
Hello,

I'm cc'ing Vittorio as I don't think that he's subscribed to the list but
he's contributed to dpxenc.c and recent colorspace filters. The same with
Kate Murray from the Library of Congress who knows a lot more about DPX
than me. Apologies if this is inappropriate.

I mostly based this patch on other ffmpeg encoders, such as pncenc.c. I'm
not really a C coder, I'm a moving image archivist who needs to be able to
specify colour metadata in DPX for various workflows. Please excuse my
ignorance/mistakes.

This patch adds documentation and two command line options for the DPX
encoder:
-trc (Transfer Characteristics) and -clr (Colorimetric Specification),
which set colour metadata values in a DPX file. Currently these are
hardcoded to always be 2, aka Linear. Ticket #6023 is related to this, but
there have also been many mailing list posts about this issue:
https://ffmpeg.org/pipermail/ffmpeg-user/2015-March/025630.html
https://ffmpeg.org/pipermail/ffmpeg-user/2015-December/029456.html

I've kept the default values as 2 (Linear) as this is what was originally
in dpxenc, but I'm not sure of the value of this really. I think that
there's more value in a default of 0 (User-defined) which would just leave
the values unspecified. Or perhaps no value at all! The initial default of
2 for colorimetric was potentially useless as 2 is listed as 'Not
applicable' for colorimetric specification in SMPTE 268M-2003.

The values for each of these options are the integers listed in the SMPTE
standards doc:
https://web.archive.org/web/20050706060025/http://www.smpte.org/smpte_store/standards/pdf/s268m.pdf

Initially I just had one argument that set the Transfer Characteristic and
Colorimetric Specification to the same value, but perhaps some use cases
could require that these values  be different? I'm not sure if they ever
would. I have never seen real world files that suggest this but I can edit
this if it seems weird.

Some of the values from 0-12 are listed as 'Not applicable' for the
colorimetric specification, but I didn't know how to specify just those
numbers (0-1, 4-10) in the patch. Perhaps it's OK to leave it as is,
otherwise hopefully someone can point me to similar code that I can learn
from. Again, apologies for my ignorance.

I'm attaching the patch and pasting it here too:

From 8ae63b8301e6822686a7885202938fd6e4cba6f2 Mon Sep 17 00:00:00 2001
From: Kieran O'Leary <kieran.o.leary@gmail.com>
Date: Wed, 1 Feb 2017 12:06:38 +0000
Subject: [PATCH] avcodec/dpxenc: support colour metadata in DPX encoder,
fixes
 ticket #6023

---
 doc/encoders.texi   | 77
+++++++++++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/dpxenc.c | 25 ++++++++++++++---
 2 files changed, 99 insertions(+), 3 deletions(-)

s->bits_per_component == 12) ?
                        1 : 0); /* packing method */
@@ -275,7 +280,20 @@ static int encode_frame(AVCodecContext *avctx,
AVPacket *pkt,

     return 0;
 }
+#define OFFSET(x) offsetof(DPXContext, x)
+#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
+static const AVOption options[] = {
+    {"trc", "Transfer Characteristics", OFFSET(transfer_characteristic),
AV_OPT_TYPE_INT, {.i64 = 2}, 0, 12, VE },
+    {"clr", "Colorimetric Specification",
OFFSET(colorimetric_specification), AV_OPT_TYPE_INT, {.i64 = 2}, 0, 12, VE
},
+    { NULL},
+};

+static const AVClass dpxenc_class = {
+    .class_name = "DPX encoder",
+    .item_name  = av_default_item_name,
+    .option     = options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
 AVCodec ff_dpx_encoder = {
     .name           = "dpx",
     .long_name      = NULL_IF_CONFIG_SMALL("DPX (Digital Picture Exchange)
image"),
@@ -293,4 +311,5 @@ AVCodec ff_dpx_encoder = {
         AV_PIX_FMT_GBRP10LE, AV_PIX_FMT_GBRP10BE,
         AV_PIX_FMT_GBRP12LE, AV_PIX_FMT_GBRP12BE,
         AV_PIX_FMT_NONE},
-};
+    .priv_class     = &dpxenc_class,
+};
\ No newline at end of file

Comments

Carl Eugen Hoyos Feb. 1, 2017, 4:37 p.m.
2017-02-01 13:42 GMT+01:00 Kieran O Leary <kieran.o.leary@gmail.com>:

> +static const AVOption options[] = {
> +    {"trc", "Transfer Characteristics", OFFSET(transfer_characteristic),
> AV_OPT_TYPE_INT, {.i64 = 2}, 0, 12, VE },
> +    {"clr", "Colorimetric Specification",
> OFFSET(colorimetric_specification), AV_OPT_TYPE_INT, {.i64 = 2}, 0, 12, VE
> },

I would have expected that the values that are written into
the output files come from AVCodecContext->color_trc
and AVCodecContext->color_primaries, not from codec
specific options.

Carl Eugen
James Almer Feb. 1, 2017, 5:01 p.m.
On 2/1/2017 9:42 AM, Kieran O Leary wrote:
> Hello,
> 
> I'm cc'ing Vittorio as I don't think that he's subscribed to the list but
> he's contributed to dpxenc.c and recent colorspace filters. The same with
> Kate Murray from the Library of Congress who knows a lot more about DPX
> than me. Apologies if this is inappropriate.
> 
> I mostly based this patch on other ffmpeg encoders, such as pncenc.c. I'm
> not really a C coder, I'm a moving image archivist who needs to be able to
> specify colour metadata in DPX for various workflows. Please excuse my
> ignorance/mistakes.
> 
> This patch adds documentation and two command line options for the DPX
> encoder:
> -trc (Transfer Characteristics) and -clr (Colorimetric Specification),

I may be missing something and apologizes if it was already mentioned, but
libavcodec already have options for these, color_trc and if i'm reading
this right colorspace, making use of the AVColorTransferCharacteristic and
AVColorSpace enums defined in avutil's pixfmt.h header.

Can't you use them instead?

> which set colour metadata values in a DPX file. Currently these are
> hardcoded to always be 2, aka Linear. Ticket #6023 is related to this, but
> there have also been many mailing list posts about this issue:
> https://ffmpeg.org/pipermail/ffmpeg-user/2015-March/025630.html
> https://ffmpeg.org/pipermail/ffmpeg-user/2015-December/029456.html
Kieran O Leary Feb. 1, 2017, 10:37 p.m.
Hi James and Carl,

Thanks for getting back to me. It looks like your comments are similar,
though you might be asking to pull the information from different places?
more below

On Wed, Feb 1, 2017 at 5:01 PM, James Almer <jamrial@gmail.com> wrote:

>
> I may be missing something and apologizes if it was already mentioned, but
> libavcodec already have options for these, color_trc and if i'm reading
> this right colorspace, making use of the AVColorTransferCharacteristic and
> AVColorSpace enums defined in avutil's pixfmt.h header.
>
> Can't you use them instead?
>
>
>
DPX contains some values that are super-specific to film, such as 'Printing
Density'. I do not see that value in either of those enums but it's a
common value that pops up in film scans.
The larger issue that I see with this approach is that it's an integer that
needs to be written to the 801/802 offsets in the DPX header. So if I want
logarithmic transfer characteristic, 3 needs to be written as the value and
this is interpreted by the decoder. Logarithmic is listed as 9 in
 AVColorTransferCharacteristic - but perhaps this is irrelevant and I'm
misunderstanding how pixfmt.h works? Things like 'unspecified video' are
not in those enums either.

Carl Eugen mentioned AvCodecContext but I don't know enough about this to
be able to see if the values as specified by SMPTE will map.

-Kieran.
Carl Eugen Hoyos Feb. 2, 2017, 8:19 a.m.
2017-02-01 23:37 GMT+01:00 Kieran O Leary <kieran.o.leary@gmail.com>:

> DPX contains some values that are super-specific to film, such as 'Printing
> Density'. I do not see that value in either of those enums but it's a
> common value that pops up in film scans.

Then I believe you have to add this property to the enums.

> The larger issue that I see with this approach is that it's an integer that
> needs to be written to the 801/802 offsets in the DPX header. So if I want
> logarithmic transfer characteristic, 3 needs to be written as the value and
> this is interpreted by the decoder. Logarithmic is listed as 9 in
>  AVColorTransferCharacteristic

This means you need a translation in dpxenc.c.

Carl Eugen
Kieran O Leary Feb. 2, 2017, 8:47 a.m.
Hi

On 2 Feb 2017 08:20, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:

2017-02-01 23:37 GMT+01:00 Kieran O Leary <kieran.o.leary@gmail.com>:

> DPX contains some values that are super-specific to film, such as
'Printing
> Density'. I do not see that value in either of those enums but it's a
> common value that pops up in film scans.

Then I believe you have to add this property to the enums.


Will do. In a separate patch?


> The larger issue that I see with this approach is that it's an integer
that
> needs to be written to the 801/802 offsets in the DPX header. So if I want
> logarithmic transfer characteristic, 3 needs to be written as the value
and
> this is interpreted by the decoder. Logarithmic is listed as 9 in
>  AVColorTransferCharacteristic

This means you need a translation in dpxenc.c.


Ah,  great. Thanks for the heads up. I'll prepare another patch once I
figure this out.

Thanks for your help!

-Kieran
Vittorio Giovara Feb. 3, 2017, 12:57 p.m.
On Wed, Feb 1, 2017 at 1:42 PM, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
> Hello,
>
> I'm cc'ing Vittorio as I don't think that he's subscribed to the list but
> he's contributed to dpxenc.c and recent colorspace filters. The same with
> Kate Murray from the Library of Congress who knows a lot more about DPX than
> me. Apologies if this is inappropriate.
>
> I mostly based this patch on other ffmpeg encoders, such as pncenc.c. I'm
> not really a C coder, I'm a moving image archivist who needs to be able to
> specify colour metadata in DPX for various workflows. Please excuse my
> ignorance/mistakes.
>
> This patch adds documentation and two command line options for the DPX
> encoder:
> -trc (Transfer Characteristics) and -clr (Colorimetric Specification), which
> set colour metadata values in a DPX file. Currently these are hardcoded to
> always be 2, aka Linear. Ticket #6023 is related to this, but there have
> also been many mailing list posts about this issue:
> https://ffmpeg.org/pipermail/ffmpeg-user/2015-March/025630.html
> https://ffmpeg.org/pipermail/ffmpeg-user/2015-December/029456.html
>
> I've kept the default values as 2 (Linear) as this is what was originally in
> dpxenc, but I'm not sure of the value of this really. I think that there's
> more value in a default of 0 (User-defined) which would just leave the
> values unspecified. Or perhaps no value at all! The initial default of 2 for
> colorimetric was potentially useless as 2 is listed as 'Not applicable' for
> colorimetric specification in SMPTE 268M-2003.
>
> The values for each of these options are the integers listed in the SMPTE
> standards doc:
> https://web.archive.org/web/20050706060025/http://www.smpte.org/smpte_store/standards/pdf/s268m.pdf
>
> Initially I just had one argument that set the Transfer Characteristic and
> Colorimetric Specification to the same value, but perhaps some use cases
> could require that these values  be different? I'm not sure if they ever
> would. I have never seen real world files that suggest this but I can edit
> this if it seems weird.
>
> Some of the values from 0-12 are listed as 'Not applicable' for the
> colorimetric specification, but I didn't know how to specify just those
> numbers (0-1, 4-10) in the patch. Perhaps it's OK to leave it as is,
> otherwise hopefully someone can point me to similar code that I can learn
> from. Again, apologies for my ignorance.
>

Hey Kieran,
I think the code looks fine. I am just wondering if we should also
offer the possibility to set these flags from the standard context
options (-color_trc and others). I'm aware that not all values match
or are valid but maybe a small conversion table or extending the main
table could be a viable approach. Similarly this could be done for the
decoder so that color properties are not lost during a dpx->dpx
conversion maybe.
Kieran O Leary Feb. 3, 2017, 1:10 p.m.
Hi Vittorio!

thanks for getting back to me.

On Fri, Feb 3, 2017 at 12:57 PM, Vittorio Giovara <
vittorio.giovara@gmail.com> wrote:

>
>
> Hey Kieran,
> I think the code looks fine. I am just wondering if we should also
> offer the possibility to set these flags from the standard context
> options (-color_trc and others). I'm aware that not all values match
> or are valid but maybe a small conversion table or extending the main
> table could be a viable approach. Similarly this could be done for the
> decoder so that color properties are not lost during a dpx->dpx
> conversion maybe.
>

That seems to be the general consensus from the replies from James Almer
and Carl Eugen and it's what i should push towards.
I added the new values locally to pixfmt.h. I'm thinking that these could
be called in a similar way to the EXR decoder? https://github.com/
FFmpeg/FFmpeg/blob/8a1759ad46f05375c957f33049b459
2befbcb224/libavcodec/exr.c#L1840

In terms of translation tables, could you point me to some simlar code that
could serve as a starting point for me? The nearest that made sense to me
seems to be these values in vf_colorpsace.c
https://github.com/FFmpeg/FFmpeg/blob/master/libavfilter/vf_colorspace.c#L97
?

-kieran
Reto Kromer Feb. 3, 2017, 1:34 p.m.
Vittorio Giovara wrote:

>I think the code looks fine. I am just wondering if we
>should also offer the possibility to set these flags from 
>the standard context options (-color_trc and others). I'm
>aware that not all values match or are valid but maybe a
>small conversion table or extending the main table could be
>a viable approach. Similarly this could be done for the
>decoder so that color properties are not lost during a
>dpx->dpx conversion maybe.

+1

In my opinion, this is important. I guess to implement an
additional conversion table would be the best solution.

Best regards, Reto
Vittorio Giovara Feb. 3, 2017, 2:46 p.m.
On Fri, Feb 3, 2017 at 2:10 PM, Kieran O Leary <kieran.o.leary@gmail.com> wrote:
> Hi Vittorio!
>
> thanks for getting back to me.
>
> On Fri, Feb 3, 2017 at 12:57 PM, Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
>>
>>
>>
>> Hey Kieran,
>> I think the code looks fine. I am just wondering if we should also
>> offer the possibility to set these flags from the standard context
>> options (-color_trc and others). I'm aware that not all values match
>> or are valid but maybe a small conversion table or extending the main
>> table could be a viable approach. Similarly this could be done for the
>> decoder so that color properties are not lost during a dpx->dpx
>> conversion maybe.
>
>
> That seems to be the general consensus from the replies from James Almer and
> Carl Eugen and it's what i should push towards.
> I added the new values locally to pixfmt.h. I'm thinking that these could be
> called in a similar way to the EXR decoder?
> https://github.com/FFmpeg/FFmpeg/blob/8a1759ad46f05375c957f33049b4592befbcb224/libavcodec/exr.c#L1840

Not sure to which changes you mean, all the values listed by that
commit are already supported by the current
AVColorTransferCharacteristic implementation. Incidentally, I believe
that the codec you point to is a perfect examples of something
libavcodec should not do, color conversion in a decoder: in my opinion
this a task that should be reserved for something external such as
lavfi or ideally lsws.

> In terms of translation tables, could you point me to some simlar code that
> could serve as a starting point for me? The nearest that made sense to me
> seems to be these values in vf_colorpsace.c
> https://github.com/FFmpeg/FFmpeg/blob/master/libavfilter/vf_colorspace.c#L97
> ?

I didn't explain myself well enough, I was mainly suggesting that
rather than having a private option used to tag the final output, you
could *also* read the standard command line option in avctx->color_trc
and use it to tag the output. Since you can't reuse the value because
dpx seem to use a different table, you should translate the value from
AVColorTransferCharacteristic to whatever dpx accepts. In pseudocode

if (priv_trc_opt = "") {
   if (avctx->color_trc == AVCOL_BT709)
      buf[801] = DPX_BT709
   else if (avctx->color_trc == AVCOL_BT601)
      buf[801] = DPX_BT601
   ...
} else
   buf[801] = priv_trc_opt
Carl Eugen Hoyos Feb. 3, 2017, 2:50 p.m.
2017-02-03 15:46 GMT+01:00 Vittorio Giovara <vittorio.giovara@gmail.com>:

> if (priv_trc_opt = "") {

Why is this useful?

>    if (avctx->color_trc == AVCOL_BT709)
>       buf[801] = DPX_BT709
>    else if (avctx->color_trc == AVCOL_BT601)
>       buf[801] = DPX_BT601

I would suggest a switch()

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/doc/encoders.texi b/doc/encoders.texi
index 8137465..d3d8eb2 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1269,6 +1269,83 @@  disabled
 A description of some of the currently available video encoders
 follows.

+@section dpx
+
+DPX image encoder.
+
+@subsection Options
+
+@table @option
+@item trc @var{integer}
+Set the transfer characteristics as listed in Table 5A in SMPTE 268M-2003:
+
+@table @samp
+@item 0
+User Defined
+@item 1
+Printing Density
+@item 2
+Linear
+@item 3
+Logarithmic [to be defined by SMPTE I23 Technology Committee, sub-group on
“Transfer Characteristics”]
+@item 4
+Unspecified Video
+@item 5
+SMPTE 274M
+@item 6
+ITU-R 709-4
+@item 7
+ITU-R 601-5 system B or G (625)
+@item 8
+ITU-R 601-5 system M (525)
+@item 9
+Composite video (NTSC); see SMPTE 170M
+@item 10
+Composite video (PAL); see ITU-R 624-4
+@item 11
+Z (depth) – linear
+@item 12
+Z (depth) – homogeneous (distance to screen and angle of view must also be
specified in user-defined section)
+@end table
+
+Default value is @var{2}.
+
+@item clr @var{integer}
+Set the Colorimetric Specification as listed in Table 5B in SMPTE
268M-2003:
+
+@table @samp
+@item 0
+User Defined
+@item 1
+Printing Density
+@item 2
+Not applicable
+@item 3
+Not Applicable
+@item 4
+Unspecified Video
+@item 5
+SMPTE 274M
+@item 6
+ITU-R 709-4
+@item 7
+ITU-R 601-5 system B or G (625)
+@item 8
+ITU-R 601-5 system M (525)
+@item 9
+Composite video (NTSC); see SMPTE 170M
+@item 10
+Composite video (PAL); see ITU-R 624-4
+@item 11
+Not applicable
+@item 12
+Not applicable
+@end table
+
+Default value is @var{2}.
+
+@end table
+
 @section Hap

 Vidvox Hap video encoder.
diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c
index a596033..3b0e890 100644
--- a/libavcodec/dpxenc.c
+++ b/libavcodec/dpxenc.c
@@ -24,15 +24,20 @@ 
 #include "libavutil/imgutils.h"
 #include "avcodec.h"
 #include "internal.h"
+#include "libavutil/opt.h"

 typedef struct DPXContext {
+    AVClass *class;
     int big_endian;
     int bits_per_component;
     int num_components;
     int descriptor;
     int planar;
+    int transfer_characteristic;
+    int colorimetric_specification;
 } DPXContext;

+
 static av_cold int encode_init(AVCodecContext *avctx)
 {
     DPXContext *s = avctx->priv_data;
@@ -218,8 +223,8 @@  static int encode_frame(AVCodecContext *avctx, AVPacket
*pkt,
     write32(buf + 772, avctx->width);
     write32(buf + 776, avctx->height);
     buf[800] = s->descriptor;
-    buf[801] = 2; /* linear transfer */
-    buf[802] = 2; /* linear colorimetric */
+    buf[801] = s->transfer_characteristic;
+    buf[802] = s->colorimetric_specification;
     buf[803] = s->bits_per_component;
     write16(buf + 804, (s->bits_per_component == 10 ||