| Message ID | 1d1e3171-771f-7e1d-0dd6-6bf04430ba5c@mediaarea.net |
|---|---|
| State | Superseded |
| Headers | show |
Hi Jerome,
Thank you so much for this patch.
I ran a quick test and ffmpeg and ffplay seems to decode these files
just fine. I noticed that taking a short 12-bit sequence (i didn't
even realise that they were big endian actually) from the Blackmagic
Cintel Scanner turned into FFV1 version 3 in Matroska with identical
framemd5s. I did get some past duration errors in the terminal though:
Here's a few frames that I used in this example. It seems to happen at
the 16th frame..
https://www.dropbox.com/sh/imaasud6yora8me/AAAXXGIQ9w6fFfvx9qigizyVa?dl=0https://www.dropbox.com/sh/imaasud6yora8me/AAAXXGIQ9w6fFfvx9qigizyVa?dl=0
./ffmpeg -start_number 00086400 -i
first_frames/99075c82-2770-4020-a289-d191dab0b852_1_%08d.dpx -c:v ffv1
-level 3 out.mkv
ffmpeg version N-89977-gddd851f Copyright (c) 2000-2018 the FFmpeg developers
built with Apple LLVM version 8.0.0 (clang-800.0.42.1)
configuration:
libavutil 56. 7.100 / 56. 7.100
libavcodec 58. 10.100 / 58. 10.100
libavformat 58. 9.100 / 58. 9.100
libavdevice 58. 1.100 / 58. 1.100
libavfilter 7. 11.101 / 7. 11.101
libswscale 5. 0.101 / 5. 0.101
libswresample 3. 0.101 / 3. 0.101
Input #0, image2, from
'first_frames/99075c82-2770-4020-a289-d191dab0b852_1_%08d.dpx':
Duration: 00:00:00.92, start: 0.000000, bitrate: N/A
Stream #0:0: Video: dpx, gbrp12le, 2160x1702 [SAR 1:1 DAR
1080:851], 24 tbr, 25 tbn, 24 tbc
Stream mapping:
Stream #0:0 -> #0:0 (dpx (native) -> ffv1 (native))
Press [q] to stop, [?] for help
[ffv1 @ 0x7fd89c013800] bits_per_raw_sample > 8, forcing range coder
Output #0, matroska, to 'out.mkv':
Metadata:
encoder : Lavf58.9.100
Stream #0:0: Video: ffv1 (FFV1 / 0x31564646), gbrp12le, 2160x1702
[SAR 1:1 DAR 1080:851], q=2-31, 200 kb/s, 24 fps, 1k tbn, 24 tbc
Metadata:
encoder : Lavc58.10.100 ffv1
frame= 2 fps=0.0 q=-0.0 size= 11653kB time=00:00:00.04
bitrate=2220121.7kbiframe= 4 fps=3.4 q=-0.0 size= 34930kB
time=00:00:00.12 bitrate=2270993.5kbiframe= 6 fps=3.4 q=-0.0 size=
58259kB time=00:00:00.20 bitrate=2283544.4kbiframe= 8 fps=3.3
q=-0.0 size= 81588kB time=00:00:00.29 bitrate=2281121.4kbiframe=
10 fps=3.3 q=-0.0 size= 104852kB
time=00:00:00.37bitrate=2284434.3kbiframe= 12 fps=3.3 q=-0.0 size=
128130kB time=00:00:00.45 bitrate=2286805.8kbiframe= 14 fps=3.3
q=-0.0 size= 151452kB time=00:00:00.54 bitrate=2284891.4kbiframe=
16 fps=3.3 q=-0.0 size= 174741kB time=00:00:00.62
bitrate=2286710.7kbiPast duration 0.639992 too large
Past duration 0.679985 too large
frame= 18 fps=3.2 q=-0.0 size= 198043kB time=00:00:00.70
bitrate=2288247.4kbiPast duration 0.719994 too large
Past duration 0.759987 too large
frame= 20 fps=3.2 q=-0.0 size= 221335kB time=00:00:00.79
bitrate=2286478.6kbiPast duration 0.799995 too large
Past duration 0.839989 too large
frame= 22 fps=3.2 q=-0.0 size= 244648kB time=00:00:00.87
bitrate=2287850.0kbiPast duration 0.879997 too large
frame= 23 fps=3.1 q=-0.0 size= 256309kB time=00:00:00.91
bitrate=2287235.2kbiframe= 23 fps=3.1 q=-0.0 Lsize= 267948kB
time=00:00:00.91 bitrate=2391103.3kbits/s speed=0.123x
video:267947kB audio:0kB subtitle:0kB other streams:0kB global
headers:0kB muxing overhead: 0.000610%
Best,
Kieran O'Leary
IFI Irish Film Archive
2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > Currently RGB and RGBA 12-bit are supported by DPX decoder only if component > values are padded (packing "Filled to 32-bit words, method A"). > This patch adds decoding of RGB > and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). I suggest we wait with this patch until we verified if RGBA dpx decoding is broken (I suspect so, I believe you have a sample to verify). As-is, we wouldn't do anybody a favour. Thank you, Carl Eugen
On 09/02/2018 02:19, Carl Eugen Hoyos wrote: > 2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: >> Currently RGB and RGBA 12-bit are supported by DPX decoder only if component >> values are padded (packing "Filled to 32-bit words, method A"). >> This patch adds decoding of RGB >> and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). > I suggest we wait with this patch until we verified if RGBA dpx decoding > is broken (I suspect so, I believe you have a sample to verify). Sorry, I see a line was missing in my first post, the one with the link to the RGBA test file. Test file for RGBA 12-bit packed: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE Regression test done on checkerboard_1080p_nuke_bigendian_12bit_alpha.dpx and checkerboard_1080p_nuke_littleendian_12bit_alpha.dpx in https://samples.ffmpeg.org/image-samples/dpx_samples.zip > > As-is, we wouldn't do anybody a favour. > > Thank you, Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2018-02-09 7:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > On 09/02/2018 02:19, Carl Eugen Hoyos wrote: >> >> 2018-02-08 11:28 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: >>> >>> Currently RGB and RGBA 12-bit are supported by DPX decoder only if >>> component >>> values are padded (packing "Filled to 32-bit words, method A"). >>> This patch adds decoding of RGB >>> and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). >> >> I suggest we wait with this patch until we verified if RGBA dpx decoding >> is broken (I suspect so, I believe you have a sample to verify). > > > Sorry, I see a line was missing in my first post, the one with the link to > the RGBA test file. > Test file for RGBA 12-bit packed: > https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE Is this a sample from a four-channel film scanner (with infrared channel)? Thank you, Carl Eugen
2018-02-09 11:15 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>: > 2018-02-09 7:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: >> Sorry, I see a line was missing in my first post, the one with the link to >> the RGBA test file. >> Test file for RGBA 12-bit packed: >> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE > > Is this a sample from a four-channel film scanner (with infrared channel)? No, it is not, please mention ticket #5639 in the commit message. Carl Eugen
On 09/02/2018 11:15, Carl Eugen Hoyos wrote: > >> Sorry, I see a line was missing in my first post, the one with the link to >> the RGBA test file. >> Test file for RGBA 12-bit packed: >> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE > Is this a sample from a four-channel film scanner (with infrared channel)? I think the question is out of topic for this patch proposal, as this question is global for all flavors of DPX (also the ones already supported by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A, here I just add the support for packed content, without adding anything else about alpha support in FFmpeg, this patch changes nothing about alpha support in FFmpeg and/or DPX) and the need for validating such patch is about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A being already available) and not for the content itself. If it is blocking for the patch inclusion, I could remove the line permitting RGBA Packed decoding.
2018-02-09 11:34 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > On 09/02/2018 11:15, Carl Eugen Hoyos wrote: >> >> >>> Sorry, I see a line was missing in my first post, the one with the link >>> to >>> the RGBA test file. >>> Test file for RGBA 12-bit packed: >>> >>> https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE >> >> Is this a sample from a four-channel film scanner (with infrared channel)? > > > I think the question is out of topic for this patch proposal, as this > question is global for all flavors of DPX (also the ones already supported > by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A, > here I just add the support for packed content, without adding anything else > about alpha support in FFmpeg, this patch changes nothing about alpha > support in FFmpeg and/or DPX) and the need for validating such patch is > about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A > being already available) and not for the content itself. Sorry, I thought you had access to such a sample and we would just have to test it, no? One more minor issue: > + } > + else { This is getting very common now (do you know why?), please fix it. Thank you, Carl Eugen
On 09/02/2018 11:39, Carl Eugen Hoyos wrote: > >> I think the question is out of topic for this patch proposal, as this >> question is global for all flavors of DPX (also the ones already supported >> by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A, >> here I just add the support for packed content, without adding anything else >> about alpha support in FFmpeg, this patch changes nothing about alpha >> support in FFmpeg and/or DPX) and the need for validating such patch is >> about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A >> being already available) and not for the content itself. > Sorry, I thought you had access to such a sample and we would > just have to test it, no? https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx is indicated by the person who provided it as with DPX alpha channel used for actually storing infrared
2018-02-12 20:47 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx > is indicated by the person who provided it as with DPX alpha channel used > for actually storing infrared Thank you! This sample appears to confirm that GraphicsMagick is right and FFmpeg is buggy. To avoid creating more incorrect (ffv1) encodes, I (strongly) suggest to first fix this issue and commit your patch to support more bit-depths but if you disagree, please feel free to commit! There is also this sample though: http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/ The only solution I can think of is to change the semantics of the fourth dpx layer by default and to add an option (to the decoder) that allows using the current semantics that defaults to "auto" reading the encoder value. Carl Eugen
On 12/02/2018 22:37, Carl Eugen Hoyos wrote: > 2018-02-12 20:47 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > >> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx >> is indicated by the person who provided it as with DPX alpha channel used >> for actually storing infrared > Thank you! > > This sample appears to confirm that GraphicsMagick is right and FFmpeg > is buggy. To avoid creating more incorrect (ffv1) encodes, I (strongly) > suggest to first fix this issue and commit your patch to support more > bit-depths but if you disagree, please feel free to commit! Sorry but I don't get it: the patch in this thread is totally separated from the alpha channel meaning issue. What should I "commit" about which "issue"? The only issues I have for the moment are that 12-bit padded DPX is supported by FFmpeg but not 12-bit packed DPX (the patch solves it), and that FFV1 support is with e.g. 12-bit YUVA but not 12-bit RGBA (I'll send a patch tomorrow for that, separated issue). I am not against continuing the discussion about the alpha channel meaning issue in a global manner, but I wish it is not blocking for this patch inclusion (I already sent the modified patch in order to fix the issues you indicated), as this patch does not create something new compared to what already exists (RGBA 8-bit, 16-bit, 10-bit padded, 12-bit padded, are already parsed by FFmpeg DPX parser) and the patch may be useful for people using "correctly" the alpha channel as they do for the other flavors of DPX, as well for people using RGB 12-bit packed. Let me know if I should split the patch in 2 (RGB part, RGBA part), so at least the RGB one could be included, as it is not related with any "alpha channel" stuff. > There is also this sample though: > http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/ > > The only solution I can think of is to change the semantics of the fourth > dpx layer by default and to add an option (to the decoder) that allows using > the current semantics that defaults to "auto" reading the encoder value. IMO having the default everywhere as currently set is not "buggy", but a global (not limited to DPX) option could be interesting for setting the semantics of the channel flagged as "alpha", because people may already use DPX or FFV1 (FFV1 supports RGBA 8-bit or YUVA 16-bit for a long time, nothing new) or any other format with alpha channel not having the "right" semantics (and whatever is the format, it is impossible to know how it is used)
2018-02-12 23:38 GMT+01:00 Jerome Martinez <jerome@mediaarea.net>: > On 12/02/2018 22:37, Carl Eugen Hoyos wrote: >> The only solution I can think of is to change the semantics of the fourth >> dpx layer by default and to add an option (to the decoder) that allows >> using the current semantics that defaults to "auto" reading the encoder >> value. > > IMO having the default everywhere as currently set is not "buggy", but a > global (not limited to DPX) option could be interesting I don't think this is a "global" issue, afaict, this is 100% dpx-specific. I also don't think a global flag (or similar) that indicates that alpha is actually infrared would me sense: GraphicsMagick (that does not show the issue that FFmpeg has) does not have such a flag and also supports a number of image formats that support alpha. Carl Eugen
diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 1aa2cbd1c8..aaacd243c9 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -65,6 +65,38 @@ static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf, return *lbuf & 0x3FF; } +static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf, + int * n_datum, int is_big) +{ + if (*n_datum) + (*n_datum)--; + else { + *lbuf = read32(ptr, is_big); + *n_datum = 7; + } + + switch (*n_datum){ + case 7: return *lbuf & 0xFFF; + case 6: return (*lbuf >> 12) & 0xFFF; + case 5: { + uint32_t c = *lbuf >> 24; + *lbuf = read32(ptr, is_big); + c |= *lbuf << 8; + return c & 0xFFF; + } + case 4: return (*lbuf >> 4) & 0xFFF; + case 3: return (*lbuf >> 16) & 0xFFF; + case 2: { + uint32_t c = *lbuf >> 28; + *lbuf = read32(ptr, is_big); + c |= *lbuf << 4; + return c & 0xFFF; + } + case 1: return (*lbuf >> 8) & 0xFFF; + default: return *lbuf >> 20; + } +} + static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, @@ -201,10 +233,29 @@ static int decode_frame(AVCodecContext *avctx, break; case 12: if (!packing) { - av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); - return -1; + int tested = 0; + if (endian && descriptor == 50 && (avctx->width%8) == 0) // Little endian and widths not a multiple of 8 need tests + tested = 1; + if (endian && descriptor == 51 && (avctx->width%2) == 0) // Little endian and widths not a multiple of 2 need tests + tested = 1; + if (!tested) { + av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); + return -1; + } + } + stride = avctx->width * elements; + if (packing) + stride *= 2; + else { + stride *= 3; // 12 bits are 1.5 byte so multiplied by 3 then divided by 2 + if (stride % 8) { + // Align to 32-bit boundaries (not tested) + stride /= 8; + stride++; + stride *= 8; + } + stride /= 2; } - stride = 2 * avctx->width * elements; break; case 16: stride = 2 * avctx->width * elements; @@ -349,6 +400,7 @@ static int decode_frame(AVCodecContext *avctx, (uint16_t*)ptr[2], (uint16_t*)ptr[3]}; for (y = 0; y < avctx->width; y++) { + if (packing) { if (elements >= 3) *dst[2]++ = read16(&buf, endian) >> 4; *dst[0] = read16(&buf, endian) >> 4; @@ -357,6 +409,18 @@ static int decode_frame(AVCodecContext *avctx, *dst[1]++ = read16(&buf, endian) >> 4; if (elements == 4) *dst[3]++ = read16(&buf, endian) >> 4; + } + else { + *dst[2]++ = read12in32(&buf, &rgbBuffer, + &n_datum, endian); + *dst[0]++ = read12in32(&buf, &rgbBuffer, + &n_datum, endian); + *dst[1]++ = read12in32(&buf, &rgbBuffer, + &n_datum, endian); + if (elements == 4) + *dst[3]++ = read12in32(&buf, &rgbBuffer, + &n_datum, endian); + } } for (i = 0; i < elements; i++) ptr[i] += p->linesize[i];
Currently RGB and RGBA 12-bit are supported by DPX decoder only if component values are padded (packing "Filled to 32-bit words, method A"). This patch adds decoding of RGB and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). As I have no file with line boundaries not aligned on 32-bit, I can not have good tests about the stride computing (so code about non aligned boundaries is theory) so I preferred to limit risks by decoding only if line boundaries are aligned on 32-bit words: - 8 pixels for RGB (8 pixels x 3 components x 12 bits = 288 bits = 9 x 32-bit words) - 2 pixels for RGBA (2 pixels x 4 components x 12 bits = 3 x 32-bit words) I think Little Endian parsing is fine thanks to the generic code about Big vs Little endian but I have no Little Endian test file so I also limited the decoding to Big Endian files. Would be happy to check with cases I was not able to check if someone provides files. I kept "Packing to 16bit required\n" message but interested in any suggestion about a better wording due to exceptions. My test files: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGB_12_Packed_BE Regression tests done on 12-bit content "Filled to 32-bit words, method A" in: https://samples.ffmpeg.org/image-samples/dpx_samples.zip From bf95371e3964e198e22dc545fc75706dedf9029b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net> Date: Thu, 8 Feb 2018 09:22:08 +0100 Subject: [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding Limited to widths multiple of 8 (RGB) and 2 (RGBA) due to lack of test files for such corner case --- libavcodec/dpx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 3 deletions(-)