| Message ID | CAO7v-1QYW6XPmbmY0gjaCuFBR8Bs5oURQ8aPoHgqoz=epd__hg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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
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.
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
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
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
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
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 ||
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