diff mbox

[FFmpeg-devel,RFC] avcodec/dpxenc: add option to force color transfer characteristics

Message ID 1513372929-4071-1-git-send-email-t.rapp@noa-archive.com
State New
Headers show

Commit Message

Tobias Rapp Dec. 15, 2017, 9:22 p.m. UTC 
Based on a patch by Kieran O'Leary. Fixes ticket #6023.

Open topics:
 - is there some mapping missing in color_trc_to_dpx?
 - the default for color_trc is DPX_TRC_UNDEFINED, would it be better to
   use DPX_TRC_USER_DEFINED instead?
 - do we need a separate encoder option for the colorimetric
   specification field? If yes, do we try to map the value from frame
   color_trc/color_primaries/...?

Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
---
 doc/encoders.texi   | 43 ++++++++++++++++++++++++++++
 libavcodec/dpxenc.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 tests/ref/lavf/dpx  | 12 ++++----
 3 files changed, 127 insertions(+), 8 deletions(-)

Comments

Carl Eugen Hoyos Dec. 16, 2017, 2:31 p.m. UTC | #1 
2017-12-15 22:22 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>:

> +    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc), AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED, DPX_TRC_NB-1, VE, "trc" },

This seems wrong to me, we have colour characteristics in general code.

Carl Eugen
Kieran O Leary April 11, 2018, 8:23 a.m. UTC | #2 
Hi Carl,

On Sat, Dec 16, 2017 at 2:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-12-15 22:22 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>:
>
>> +    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc), AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED, DPX_TRC_NB-1, VE, "trc" },
>
> This seems wrong to me, we have colour characteristics in general code.
>

There is a method in this patch that takes values from -color_trc, is
that not sufficient?

$ ./ffmpeg -f lavfi -i testsrc -color_trc bt709 -vframes 1 out.dpx &&
mediainfo out.dpx |tail -n 4

ffmpeg version N-90649-g9825f77 Copyright (c) 2000-2018 the FFmpeg developers

  built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

  configuration:

  libavutil      56. 13.100 / 56. 13.100

  libavcodec     58. 17.100 / 58. 17.100

  libavformat    58. 11.101 / 58. 11.101

  libavdevice    58.  2.100 / 58.  2.100

  libavfilter     7. 14.100 /  7. 14.100

  libswscale      5.  0.102 /  5.  0.102

  libswresample   3.  0.101 /  3.  0.101

Input #0, lavfi, from 'testsrc':

  Duration: N/A, start: 0.000000, bitrate: N/A

    Stream #0:0: Video: rawvideo (RGB[24] / 0x18424752), rgb24,
320x240 [SAR 1:1 DAR 4:3], 25 tbr, 25 tbn, 25 tbc

File 'out.dpx' already exists. Overwrite ? [y/N] y

Stream mapping:

  Stream #0:0 -> #0:0 (rawvideo (native) -> dpx (native))

Press [q] to stop, [?] for help

Output #0, image2, to 'out.dpx':

  Metadata:

    encoder         : Lavf58.11.101

    Stream #0:0: Video: dpx, rgb24(unknown/unknown/bt709), 320x240
[SAR 1:1 DAR 4:3], q=2-31, 200 kb/s, 25 fps, 25 tbn, 25 tbc

    Metadata:

      encoder         : Lavc58.17.100 dpx

frame=    1 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.04 bitrate=N/A
speed=20.6x

video:227kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: unknown

Color primaries                          : BT.709

Transfer characteristics                 : BT.709


Best,

Kieran.
Tobias Rapp April 11, 2018, 8:42 a.m. UTC | #3 
On 11.04.2018 10:23, Kieran O Leary wrote:
> Hi Carl,
> 
> On Sat, Dec 16, 2017 at 2:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2017-12-15 22:22 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>:
>>
>>> +    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc), AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED, DPX_TRC_NB-1, VE, "trc" },
>>
>> This seems wrong to me, we have colour characteristics in general code.
>>
> 
> There is a method in this patch that takes values from -color_trc, is
> that not sufficient?
> 
> [...]

If I understand it correctly Carl wants to have the DPX_TRC_* enum 
values merged into AVCOL_TRC_*. My feeling is that I currently don't 
have enough knowledge about all those TRC specifications to properly 
sort them into the list, and did not find time to dig into the topic. 
Also I doubt the general usefulness of DPX_TRC_USER_DEFINED or 
DPX_TRC_UNSPECIFIED_VIDEO outside of DPX encoding.

Regards,
Tobias
Kieran O Leary April 11, 2018, 8:51 a.m. UTC | #4 
On Wed, Apr 11, 2018 at 9:42 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote:
> On 11.04.2018 10:23, Kieran O Leary wrote:
>>
>> Hi Carl,
>>
>> On Sat, Dec 16, 2017 at 2:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> wrote:
>>>
>>> 2017-12-15 22:22 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>:
>>>
>>>> +    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc),
>>>> AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED,
>>>> DPX_TRC_NB-1, VE, "trc" },
>>>
>>>
>>> This seems wrong to me, we have colour characteristics in general code.
>>>
>>
>> There is a method in this patch that takes values from -color_trc, is
>> that not sufficient?
>>
>> [...]
>
>
> If I understand it correctly Carl wants to have the DPX_TRC_* enum values
> merged into AVCOL_TRC_*. My feeling is that I currently don't have enough
> knowledge about all those TRC specifications to properly sort them into the
> list, and did not find time to dig into the topic. Also I doubt the general
> usefulness of DPX_TRC_USER_DEFINED or DPX_TRC_UNSPECIFIED_VIDEO outside of
> DPX encoding.
>

I also don't see the point of the DPX specific values to be used in
the general list. I'd include Printing Density in this as well.

Best,

Kieran.
Hendrik Leppkes April 11, 2018, 8:52 a.m. UTC | #5 
On Wed, Apr 11, 2018 at 10:42 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote:
> On 11.04.2018 10:23, Kieran O Leary wrote:
>>
>> Hi Carl,
>>
>> On Sat, Dec 16, 2017 at 2:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> wrote:
>>>
>>> 2017-12-15 22:22 GMT+01:00 Tobias Rapp <t.rapp@noa-archive.com>:
>>>
>>>> +    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc),
>>>> AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED,
>>>> DPX_TRC_NB-1, VE, "trc" },
>>>
>>>
>>> This seems wrong to me, we have colour characteristics in general code.
>>>
>>
>> There is a method in this patch that takes values from -color_trc, is
>> that not sufficient?
>>
>> [...]
>
>
> If I understand it correctly Carl wants to have the DPX_TRC_* enum values
> merged into AVCOL_TRC_*. My feeling is that I currently don't have enough
> knowledge about all those TRC specifications to properly sort them into the
> list, and did not find time to dig into the topic. Also I doubt the general
> usefulness of DPX_TRC_USER_DEFINED or DPX_TRC_UNSPECIFIED_VIDEO outside of
> DPX encoding.

All the AVColor* enums track the values from ISO/IEC 23001-8, which is
also the same numeric values most major codecs follow, any values not
defined in that standard should not get into those enums at this
point.

Somwhat related to that, the two 601 values probably have mappings in AVCOL_TRC.

- Hendrik
diff mbox

Patch

diff --git a/doc/encoders.texi b/doc/encoders.texi
index 88ef8f9..f3f36bf 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1312,6 +1312,49 @@  disabled
 A description of some of the currently available video encoders
 follows.
 
+@section dpx
+
+Digital Moving-Picture Exchange (DPX) image encoder.
+
+@subsection Options
+
+@table @option
+@item dpx_color_trc @var{integer}
+DPX color transfer characteristics (see Table 5A in SMPTE 268M-2003).
+
+@table @option
+@item user
+User Defined
+@item print
+Printing Density
+@item linear
+Linear
+@item log
+Logarithmic
+@item video
+Unspecified Video
+@item smpte274m
+SMPTE-274M
+@item itur709
+ITU-R 709-4
+@item itur601bg
+ITU-R 601-5 (system B or G)
+@item itur601m
+ITU-R 601-5 (system M)
+@item ntsc
+NTSC Composite Video
+@item pal
+PAL Composite Video
+@item zlinear
+Z-Linear
+@item zhomogen
+Z-Homogeneous
+@end table
+
+Default value is derived from encoder, when mapping is available.
+
+@end table
+
 @section Hap
 
 Vidvox Hap video encoder.
diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c
index a596033..ddcd53b 100644
--- a/libavcodec/dpxenc.c
+++ b/libavcodec/dpxenc.c
@@ -22,17 +22,56 @@ 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
 #include "libavutil/imgutils.h"
+#include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
+#include "libavutil/pixfmt.h"
 #include "avcodec.h"
 #include "internal.h"
 
 typedef struct DPXContext {
+    AVClass *class;
     int big_endian;
     int bits_per_component;
     int num_components;
     int descriptor;
     int planar;
+    int color_trc;
 } DPXContext;
 
+enum DPXTransferCharacteristic {
+    DPX_TRC_UNDEFINED            = -1,
+    DPX_TRC_USER_DEFINED         = 0,
+    DPX_TRC_PRINTING_DENSITY     = 1,
+    DPX_TRC_LINEAR               = 2,
+    DPX_TRC_LOG                  = 3,   // Logarithmic
+    DPX_TRC_UNSPECIFIED_VIDEO    = 4,
+    DPX_TRC_SMPTE274M            = 5,   // SMPTE 274M
+    DPX_TRC_ITUR709              = 6,   // ITU-R 709-4
+    DPX_TRC_ITUR601BG            = 7,   // ITU-R 601-5 system B or G (625)
+    DPX_TRC_ITUR601M             = 8,   // ITU-R 601-5 system M (525)
+    DPX_TRC_COMPOSITE_VIDEO_NTSC = 9,   // Composite video (NTSC), see SMPTE 170M
+    DPX_TRC_COMPOSITE_VIDEO_PAL  = 10,  // Composite video (PAL), see ITU-R 624-4
+    DPX_TRC_Z_LINEAR             = 11,
+    DPX_TRC_Z_HOMOGENEOUS        = 12,
+    DPX_TRC_NB
+};
+
+static int color_trc_to_dpx(enum AVColorTransferCharacteristic color_trc)
+{
+    switch (color_trc) {
+    case AVCOL_TRC_LINEAR:
+        return DPX_TRC_LINEAR;
+    case AVCOL_TRC_LOG:
+        return DPX_TRC_LOG;
+    case AVCOL_TRC_BT709:
+        return DPX_TRC_ITUR709;
+    case AVCOL_TRC_SMPTE170M:
+        return DPX_TRC_COMPOSITE_VIDEO_NTSC;
+    default:
+        return DPX_TRC_UNDEFINED;
+    }
+}
+
 static av_cold int encode_init(AVCodecContext *avctx)
 {
     DPXContext *s = avctx->priv_data;
@@ -178,6 +217,7 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
 {
     DPXContext *s = avctx->priv_data;
     int size, ret, need_align, len;
+    int color_trc = s->color_trc;
     uint8_t *buf;
 
 #define HEADER_SIZE 1664  /* DPX Generic header */
@@ -200,6 +240,14 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
         return ret;
     buf = pkt->data;
 
+    if (color_trc < 0)
+        color_trc = color_trc_to_dpx(frame->color_trc);
+    if (color_trc < 0)
+        color_trc = color_trc_to_dpx(avctx->color_trc);
+    av_log(avctx, AV_LOG_TRACE, "codec_color_trc:%s frame_color_trc:%s dpx_color_trc:%d\n",
+           av_color_transfer_name(avctx->color_trc), av_color_transfer_name(frame->color_trc),
+           color_trc);
+
     memset(buf, 0, HEADER_SIZE);
 
     /* File information header */
@@ -218,8 +266,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] = color_trc; /* transfer characteristics */
+    buf[802] = color_trc; /* colorimetric specification */
     buf[803] = s->bits_per_component;
     write16(buf + 804, (s->bits_per_component == 10 || s->bits_per_component == 12) ?
                        1 : 0); /* packing method */
@@ -276,6 +324,33 @@  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[] = {
+    { "dpx_color_trc",  "Transfer Characteristics", OFFSET(color_trc), AV_OPT_TYPE_INT, { .i64 = DPX_TRC_UNDEFINED }, DPX_TRC_UNDEFINED, DPX_TRC_NB-1, VE, "trc" },
+        { "user",       "User Defined",                0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_USER_DEFINED },          0, 0, VE, "trc" },
+        { "print",      "Printing Density",            0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_PRINTING_DENSITY },      0, 0, VE, "trc" },
+        { "linear",     "Linear",                      0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_LINEAR },                0, 0, VE, "trc" },
+        { "log",        "Logarithmic",                 0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_LOG },                   0, 0, VE, "trc" },
+        { "video",      "Unspecified Video",           0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_UNSPECIFIED_VIDEO },     0, 0, VE, "trc" },
+        { "smpte274m",  "SMPTE-274M",                  0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_SMPTE274M },             0, 0, VE, "trc" },
+        { "itur709",    "ITU-R 709-4",                 0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_ITUR709 },               0, 0, VE, "trc" },
+        { "itur601bg",  "ITU-R 601-5 (system B or G)", 0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_ITUR601BG },             0, 0, VE, "trc" },
+        { "itur601m",   "ITU-R 601-5 (system M)",      0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_ITUR601M },              0, 0, VE, "trc" },
+        { "ntsc",       "NTSC Composite Video",        0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_COMPOSITE_VIDEO_NTSC },  0, 0, VE, "trc" },
+        { "pal",        "PAL Composite Video",         0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_COMPOSITE_VIDEO_PAL },   0, 0, VE, "trc" },
+        { "zlinear",    "Z-Linear",                    0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_Z_LINEAR },              0, 0, VE, "trc" },
+        { "zhomogen",   "Z-Homogeneous",               0, AV_OPT_TYPE_CONST, { .i64 = DPX_TRC_Z_HOMOGENEOUS },         0, 0, VE, "trc" },
+    { 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 +368,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,
 };
diff --git a/tests/ref/lavf/dpx b/tests/ref/lavf/dpx
index 7bbed3e..6092570 100644
--- a/tests/ref/lavf/dpx
+++ b/tests/ref/lavf/dpx
@@ -1,18 +1,18 @@ 
-4c8880d5835ffb5fe37c1ed8c8d404de *./tests/data/images/dpx/02.dpx
+f7c1f414a5ac67fb46c19094af15d88b *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0x6da01946
 305792 ./tests/data/images/dpx/02.dpx
-7ca935d5d5e00c54acbc85565d3039b6 *./tests/data/images/dpx/02.dpx
+2196b8d0a224177487438da63f5c7f86 *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0xe6663fba
 407168 ./tests/data/images/dpx/02.dpx
-a4cfea1797c928f2eff73573e559675d *./tests/data/images/dpx/02.dpx
+6596e31dca6122759601650d9b9b33b5 *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0x1c755633
 609920 ./tests/data/images/dpx/02.dpx
-075963c3c08978b6a20555ba09161434 *./tests/data/images/dpx/02.dpx
+958042039a25d5cbb9a351503bc57052 *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0xe5b9c023
 609920 ./tests/data/images/dpx/02.dpx
-b9f22728f8ff393bf30cf6cbd624fa95 *./tests/data/images/dpx/02.dpx
+7bf3bf630d5ca27d5703e5dfb5ad217c *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0xf38d5830
 407168 ./tests/data/images/dpx/02.dpx
-545603630f30dec2768c8ae8d12eb8ea *./tests/data/images/dpx/02.dpx
+414c10bfd8c8d14d1ad65ed009755ab0 *./tests/data/images/dpx/02.dpx
 ./tests/data/images/dpx/%02d.dpx CRC=0xe72ce131
 812672 ./tests/data/images/dpx/02.dpx