| Message ID | 20201007210240.11304-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/dpxenc: stop hardcoding color trc/primaries | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | fail | Make fate failed |
Woah, more amazing film preservation patches, thank you! From my uninformed reading of the code, does this only support the detection of Linear, 709, 240M, 170M, Gamm22? The reason I ask is that you frequently see Printing Density appear as well, which has a value of '1' in the 801/802 offset.. Best, Kieran O'Leary, National Library of Ireland
> On 7 Oct 2020, at 22:02, Paul B Mahol <onemda@gmail.com> wrote: > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/dpxenc.c | 36 ++++++++++++++++++++++++++++++++++-- > tests/ref/lavf/dpx | 2 +- > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c > index a5960334d5..56840a8d33 100644 > --- a/libavcodec/dpxenc.c > +++ b/libavcodec/dpxenc.c > @@ -173,6 +173,38 @@ static void encode_gbrp12(AVCodecContext *avctx, const AVFrame *pic, uint16_t *d > } > } > > +static int get_dpx_pri(int color_pri) > +{ > + switch (color_pri) { > + case AVCOL_PRI_BT709: > + return 6; > + case AVCOL_PRI_SMPTE240M: > + case AVCOL_PRI_SMPTE170M: > + return 9; I think perhaps this should be 8 (ITU 601 525), rather than 9 (Composite video SMPTE 170M), but I am not sure? > + case AVCOL_PRI_BT470BG: > + return 10; Perhaps this should be 7 (ITU 601 625), rather than 10 (ITU 624-4 Composite video PAL), again not sure which is most widely used? > + default: > + return 0; > + } > +} In DPX files containing colour difference information the colorspace would also be keyed off the value returned from this function. Perhaps it should be taken into account here (for files containing YCbCr)? > + > +static int get_dpx_trc(int color_trc) > +{ > + switch (color_trc) { > + case AVCOL_TRC_LINEAR: > + return 2; > + case AVCOL_TRC_BT709: > + return 6; > + case AVCOL_TRC_SMPTE240M: > + case AVCOL_TRC_SMPTE170M: > + return 9; This value could be 7, 8 or 9. From my reading of the spec the best might be to take colour_primaries into account and do something like: if (AVCOL_PRI_BT470BG) return 7 (ITU 601 625) else return 8 (ITU 601 525) > + case AVCOL_TRC_GAMMA22: > + return 10; 10 is ITU 624-4 Composite video PAL, which says it has gamma = 2.8 (AVCOL_TRC_GAMMA28). https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.624-4-1990-PDF-E.pdf > + default: > + return 0; > + } > +} > + > > [..] > Best, Harry
On Thu, Oct 08, 2020 at 12:27:02PM +0100, Harry Mallon wrote: > > > > On 7 Oct 2020, at 22:02, Paul B Mahol <onemda@gmail.com> wrote: > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > --- > > libavcodec/dpxenc.c | 36 ++++++++++++++++++++++++++++++++++-- > > tests/ref/lavf/dpx | 2 +- > > 2 files changed, 35 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c > > index a5960334d5..56840a8d33 100644 > > --- a/libavcodec/dpxenc.c > > +++ b/libavcodec/dpxenc.c > > @@ -173,6 +173,38 @@ static void encode_gbrp12(AVCodecContext *avctx, const AVFrame *pic, uint16_t *d > > } > > } > > > > +static int get_dpx_pri(int color_pri) > > +{ > > + switch (color_pri) { > > + case AVCOL_PRI_BT709: > > + return 6; > > + case AVCOL_PRI_SMPTE240M: > > + case AVCOL_PRI_SMPTE170M: > > + return 9; > > I think perhaps this should be 8 (ITU 601 525), rather than 9 (Composite video SMPTE 170M), but I am not sure? The smpte170m is explicitly mention in specification, so make sure you use latest version of it. > > > + case AVCOL_PRI_BT470BG: > > + return 10; > > Perhaps this should be 7 (ITU 601 625), rather than 10 (ITU 624-4 Composite video PAL), again not sure which is most widely used? see first comment. > > > + default: > > + return 0; > > + } > > +} > > In DPX files containing colour difference information the colorspace would also be keyed off the value returned from this function. Perhaps it should be taken into account here (for files containing YCbCr)? Sorry, this does not make sense, the color_prim/trc is meaningfull for both yuv and rgb. > > > + > > +static int get_dpx_trc(int color_trc) > > +{ > > + switch (color_trc) { > > + case AVCOL_TRC_LINEAR: > > + return 2; > > + case AVCOL_TRC_BT709: > > + return 6; > > + case AVCOL_TRC_SMPTE240M: > > + case AVCOL_TRC_SMPTE170M: > > + return 9; > > This value could be 7, 8 or 9. From my reading of the spec the best might be to take colour_primaries into account and do something like: > > if (AVCOL_PRI_BT470BG) return 7 (ITU 601 625) > else return 8 (ITU 601 525) > see first comment. > > + case AVCOL_TRC_GAMMA22: > > + return 10; > > 10 is ITU 624-4 Composite video PAL, which says it has gamma = 2.8 (AVCOL_TRC_GAMMA28). https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.624-4-1990-PDF-E.pdf > Hmm i will double check. > > + default: > > + return 0; > > + } > > +} > > + > > > > [..] > > > > Best, > Harry > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> >>> >>> [..] >>> +static int get_dpx_pri(int color_pri) >>> +{ >>> + switch (color_pri) { >>> + case AVCOL_PRI_BT709: >>> + return 6; >>> + case AVCOL_PRI_SMPTE240M: >>> + case AVCOL_PRI_SMPTE170M: >>> + return 9; >> >> I think perhaps this should be 8 (ITU 601 525), rather than 9 (Composite video SMPTE 170M), but I am not sure? > > The smpte170m is explicitly mention in specification, so make sure you use latest version of it. Let me try to explain myself a little better. AVCOL_PRI_SMPTE170M does not mean SMPTE170M, it means SMPTE170M, ITU-R BT601-6 525, ITU-R BT1358 525 or ITU-R BT1700 NTSC (see pixfmt.h). We will have to pick a DPX option though, which could either be 9 (SMPTE170M) or 8 (ITU-R 601 525). My guess is that the ITU 601 standard would be the most obvious choice. I guess it probably doesn't matter as the options amount to the same thing, DPX does prefer that the two bytes match in V2.0 though. > >> >>> + case AVCOL_PRI_BT470BG: >>> + return 10; >> >> Perhaps this should be 7 (ITU 601 625), rather than 10 (ITU 624-4 Composite video PAL), again not sure which is most widely used? > > see first comment. The primaries of ITU 624-4 PAL and ITU 601 625 are the same here, so choosing between 7 and 10 is difficult. I wonder whether the ITU 601 option is the least surprising. BT470BG is not explicitly mentioned in the DPX specification. > >> >>> + default: >>> + return 0; >>> + } >>> +} >> >> In DPX files containing colour difference information the colorspace would also be keyed off the value returned from this function. Perhaps it should be taken into account here (for files containing YCbCr)? > > Sorry, this does not make sense, the color_prim/trc is meaningfull for both yuv and rgb. In FFMPEG we have 3 things AVColorPrimaries, AVColorTransferCharacteristic and AVColorSpace. In DPX we have only two; Transfer Characteristic and Colorimetric Specification. If we were to read these values when decoding dpx files we would choose what to set for AVColorSpace (the YCbCr matrix) based on the Colorimetric Specification. I think that should mean that we should take the AVColorSpace and the AVColorPrimaries into consideration when choosing what to put in this byte, when writing YUV DPX files. > >> >>> + >>> +static int get_dpx_trc(int color_trc) >>> +{ >>> + switch (color_trc) { >>> + case AVCOL_TRC_LINEAR: >>> + return 2; >>> + case AVCOL_TRC_BT709: >>> + return 6; >>> + case AVCOL_TRC_SMPTE240M: >>> + case AVCOL_TRC_SMPTE170M: >>> + return 9; >> >> This value could be 7, 8 or 9. From my reading of the spec the best might be to take colour_primaries into account and do something like: >> >> if (AVCOL_PRI_BT470BG) return 7 (ITU 601 625) >> else return 8 (ITU 601 525) >> > > see first comment. See my first comment > >>> + case AVCOL_TRC_GAMMA22: >>> + return 10; >> >> 10 is ITU 624-4 Composite video PAL, which says it has gamma = 2.8 (AVCOL_TRC_GAMMA28). https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.624-4-1990-PDF-E.pdf >> > > Hmm i will double check. > >>> + default: >>> + return 0; >>> + } >>> +} >>> + >>> >>> [..] >>> >> [..]
Pinging as I think my comment got lost in the conversation, my main concern is about the Printing Density value aka 1, K On Thu, Oct 8, 2020 at 10:25 AM Kieran O Leary <kieran.o.leary@gmail.com> wrote: > Woah, more amazing film preservation patches, thank you! > From my uninformed reading of the code, does this only support the > detection of Linear, 709, 240M, 170M, Gamm22? The reason I ask is that you > frequently see Printing Density appear as well, which has a value of '1' > in the 801/802 offset.. > > Best, > > Kieran O'Leary, > National Library of Ireland >
On Fri, Oct 09, 2020 at 10:08:45AM +0100, Kieran O Leary wrote: > Pinging as I think my comment got lost in the conversation, my main concern > is about the Printing Density value aka 1, This patch is for encoder, not for decoder (well decoder also need fixing but later). The meaning of 1 value is same as 0 IMHO. > > K > > On Thu, Oct 8, 2020 at 10:25 AM Kieran O Leary <kieran.o.leary@gmail.com> > wrote: > > > Woah, more amazing film preservation patches, thank you! > > From my uninformed reading of the code, does this only support the > > detection of Linear, 709, 240M, 170M, Gamm22? The reason I ask is that you > > frequently see Printing Density appear as well, which has a value of '1' > > in the 801/802 offset.. > > > > Best, > > > > Kieran O'Leary, > > National Library of Ireland > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Oct 9, 2020 at 12:10 PM Paul B Mahol <onemda@gmail.com> wrote: > On Fri, Oct 09, 2020 at 10:08:45AM +0100, Kieran O Leary wrote: > > Pinging as I think my comment got lost in the conversation, my main > concern > > is about the Printing Density value aka 1, > > This patch is for encoder, not for decoder (well decoder also need fixing > but later). > > The meaning of 1 value is same as 0 IMHO. > http://www.simplesystems.org/users/bfriesen/dpx/S268M_Revised.pdf I know it's an older version of the spec but pretty sure new one still has a value of 1 being Printing Density in tables 5a and 5b. Best, Kieran
On Fri, Oct 09, 2020 at 01:48:46PM +0100, Kieran O Leary wrote: > On Fri, Oct 9, 2020 at 12:10 PM Paul B Mahol <onemda@gmail.com> wrote: > > > On Fri, Oct 09, 2020 at 10:08:45AM +0100, Kieran O Leary wrote: > > > Pinging as I think my comment got lost in the conversation, my main > > concern > > > is about the Printing Density value aka 1, > > > > This patch is for encoder, not for decoder (well decoder also need fixing > > but later). > > > > The meaning of 1 value is same as 0 IMHO. > > > > http://www.simplesystems.org/users/bfriesen/dpx/S268M_Revised.pdf > I know it's an older version of the spec but pretty sure new one still has > a value of 1 being Printing Density in tables 5a and 5b. I never claimed different. Issue is that there is nothing like printing density in either standardised transfer characteristic of transffer primaries.
diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c index a5960334d5..56840a8d33 100644 --- a/libavcodec/dpxenc.c +++ b/libavcodec/dpxenc.c @@ -173,6 +173,38 @@ static void encode_gbrp12(AVCodecContext *avctx, const AVFrame *pic, uint16_t *d } } +static int get_dpx_pri(int color_pri) +{ + switch (color_pri) { + case AVCOL_PRI_BT709: + return 6; + case AVCOL_PRI_SMPTE240M: + case AVCOL_PRI_SMPTE170M: + return 9; + case AVCOL_PRI_BT470BG: + return 10; + default: + return 0; + } +} + +static int get_dpx_trc(int color_trc) +{ + switch (color_trc) { + case AVCOL_TRC_LINEAR: + return 2; + case AVCOL_TRC_BT709: + return 6; + case AVCOL_TRC_SMPTE240M: + case AVCOL_TRC_SMPTE170M: + return 9; + case AVCOL_TRC_GAMMA22: + return 10; + default: + return 0; + } +} + static int encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *frame, int *got_packet) { @@ -218,8 +250,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] = get_dpx_trc(avctx->color_trc); + buf[802] = get_dpx_pri(avctx->color_primaries); buf[803] = s->bits_per_component; write16(buf + 804, (s->bits_per_component == 10 || s->bits_per_component == 12) ? 1 : 0); /* packing method */ diff --git a/tests/ref/lavf/dpx b/tests/ref/lavf/dpx index 68fe25afcd..39e513430a 100644 --- a/tests/ref/lavf/dpx +++ b/tests/ref/lavf/dpx @@ -1,3 +1,3 @@ -4c8880d5835ffb5fe37c1ed8c8d404de *tests/data/images/dpx/02.dpx +233e219cbfa61e0b77f8e4fad05b2404 *tests/data/images/dpx/02.dpx tests/data/images/dpx/%02d.dpx CRC=0x6da01946 305792 tests/data/images/dpx/02.dpx
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/dpxenc.c | 36 ++++++++++++++++++++++++++++++++++-- tests/ref/lavf/dpx | 2 +- 2 files changed, 35 insertions(+), 3 deletions(-)