Fix yuv420p to p01x unscaled conversion

[nyanmisaka]

Changes

• Fix yuv420p to p01x unscaled conversion

Issues

ffmpeg -f lavfi -i color=c=gray:s=1920x1080 \
-vf format=yuv420p,format=p010le -vframes 1 -y /tmp/yuv420p_p010.yuv \
-vf format=yuv422p,format=p010le -vframes 1 -y /tmp/yuv422p_p010.yuv \
-vf format=yuv444p,format=p010le -vframes 1 -y /tmp/yuv444p_p010.yuv \
-vf format=nv12,format=p010le -vframes 1 -y /tmp/nv12_p010.yuv \
-vf format=nv16,format=p010le -vframes 1 -y /tmp/nv16_p010.yuv \
-vf format=nv24,format=p010le -vframes 1 -y /tmp/nv24_p010.yuv

sha256sum /tmp/*_p010.yuv
8836c7ea7086f1d3c0f193c5a47ec203a57fb48d838c9d1180e252ced388f7f6  /tmp/nv12_p010.yuv
8836c7ea7086f1d3c0f193c5a47ec203a57fb48d838c9d1180e252ced388f7f6  /tmp/nv16_p010.yuv
8836c7ea7086f1d3c0f193c5a47ec203a57fb48d838c9d1180e252ced388f7f6  /tmp/nv24_p010.yuv
c4d2b32028354545ff5bd212e12def79085b9808b9623a6bc96aa72ae25604ec  /tmp/yuv420p_p010.yuv
8836c7ea7086f1d3c0f193c5a47ec203a57fb48d838c9d1180e252ced388f7f6  /tmp/yuv422p_p010.yuv
8836c7ea7086f1d3c0f193c5a47ec203a57fb48d838c9d1180e252ced388f7f6  /tmp/yuv444p_p010.yuv

(Originally reported in HandBrake/HandBrake#5011)

[gnattu]

Is this reported to ffmpeg upstream yet?

[nyanmisaka]

Is this reported to ffmpeg upstream yet?

Not seen yet. https://github.com/HandBrake/HandBrake/blob/master/contrib/ffmpeg/A12-libswscale-fix-yuv420p-to-p01xle-color-conversion-bu.patch

My email configuration has been broken for a while so it's not very convenient to send patches. Perhaps you can do it? Or send it to https://github.com/intel-media-ci/ffmpeg/pulls

[ValeZAA]

This makes md5 the same as it was before FFmpeg/FFmpeg@e3fd185

Are you sure this is not just another bug fixed with accurate_rnd? Try -sws_flags accurate_rnd.

[gnattu]

This makes md5 the same as it was before FFmpeg/FFmpeg@e3fd185

Why the md5 has to change in the first place? Shouldn't they look exactly the same for the same grayscale input?

Are you sure this is not just another bug fixed with accurate_rnd? Try -sws_flags accurate_rnd.

I'm very sure it is not. It is so wrong that you can identify that with your bare eyes. And yes, I've tested with -sws_flags accurate_rnd and that does not fix the color.

[ValeZAA]

So Intel defined P010 differently from nvidia (see ffmpeg -h encoder=hevc_nvenc, there is P010 support), https://trac.ffmpeg.org/ticket/7199#comment:16

FFmpeg/FFmpeg@e3fd185 is correct for Nvidia, but incorrect for Intel. This is the issue. There is a sample here that shows it https://trac.ffmpeg.org/ticket/8055

There is also an issue that capture cards that signal P010 also have a special flag to signal whether it is Nvidia or Intel flavour of P010 (mxfFrameInfo::Shift).

[gnattu]

I don't know how that is related to hardware formats if at all honestly. We are using ffmpeg's internal software filter to generate grayscale image, using ffmpeg internal software scale to convert colorspace, and ffmpeg's software scale produces inconsistent results. No hardware specific things are involved and it is just ffmpeg implemented its color conversion wrong.

[ValeZAA]

P010 is actually 10 bit color space, but 420, so for Y plane 10 bit and for Cb, Cr together 2 times less data 5 bits are used, so together 15 bits... Only -vf format=yuv420p10,format=p010le is probably correct. Then there is no issue, right? I cannot use YUView to see what is being produced, alas, but I can see it is 126, 126, 126 RGB with ffplay.

What you are not accounting for is that 420 has chroma siting. That means it may be a mismatch somewhere there.

But again unscaled and scaled is how Intel and Nvidia define it, I doubt it can be changed.

[gnattu]

What you are not accounting for is that 420 has chroma siting. That means it may be a mismatch somewhere there.

What you're not seeing is just the code that implements this conversion. The function directly takes 8-bit 420p inputs and outputs p010 frames without any intermediary steps. If you're implying that FFmpeg is producing yuv420p frames incorrectly at the start, that would be a much larger issue, as it would break most decoder usages that everyone uses.

But again unscaled and scaled is how Intel and Nvidia define it, I doubt it can be changed.

But again it is not related to current issue in any way.

Only -vf format=yuv420p10,format=p010le is probably correct. Then there is no issue, right?

It is using a completely different conversion function, that one is correct means nothing.

I cannot use YUView to see what is being produced, alas, but I can see it is 126, 126, 126 RGB with ffplay.

If you can use ffplay then you will notice that they are different. It is so subtle that you don't need a color meter at all.

[nyanmisaka]

yuv420p (fully-planar) => p010le (semi-planar)

---

Without the fast path:

plane Y ==copy==> Chroma (internal) ==copy==> plane Y

plane U ==copy==> LumaU (internal) \
                                    ==copy==> plane UV (interlaced)
plane V ==copy==> LumaV (internal) /

---

With the fast path:

plane Y  ==copy==> plane Y

plane U \
         ==copy==> plane UV (interlaced)
plane V /

The purpose of the fast path planar8ToP01xleWrapper() is to reduce an internal copy to improve performance, rather than changing the pixel values.

[gnattu]

If you still insist upstream did it correctly and that is due to hardware vendor stuff, let me make things clear for you as it seems like you don't familiar with those functions and how frames format differs.

What you are keep talking about the P010 difference, is the difference whether to store the 10bits in the high 10 bits of the 16bits, or the low 10 bits of the 16bits, and it has nothing to do with this color conversion function, as it always store in the high 10 bits.

What was incorrect was that the low 12 bits would contain garbage, especially the low 2 bits of the 10 bit color and it makes a subtle difference on grayscale images.

You have to pick your poison: all other 8 to 10 bit conversion functions are wrong, or this specific conversion is wrong.

[ValeZAA]

You are right. Y plane only format (which is what gray is) is not affected by chroma siting. So clearly must be a bug.

And you are right that 10 bit is using different code path.

P.S. Patch applied upstream FFmpeg/FFmpeg@0bb5394