erazortt / DoViBaker

Bake the DoVi into your clip
GNU General Public License v3.0
41 stars 7 forks source link

Fix crash when both BL and EL have different subsampling #2

Closed Asd-g closed 1 year ago

Asd-g commented 1 year ago

This should fix the crash when both BL and EL have different subsampling.

Btw there is vertical chroma shift when BL is 420.

erazortt commented 1 year ago

So you say there is vertical chorma shift when BL is 420. Does this mean that that bl chroma is shifted against the bl luma, or is the el chroma shifted? Does this depend on the el subsampling?

Asd-g commented 1 year ago

For BL 420 - bl chroma is shifted against the bl luma and it's visible on the output. You can check images 1, 2, 3, 4. I marked red light where the shift is obvious when zoomed 4-6x.

Actually the same vertical shift is valid for EL-420 too but it's not visible on the final output. You can check images 5, 6.

Images - https://files.catbox.moe/3y4g7z.7z

Edit: BL-420 chroma shift is not related to EL and EL-420 shift is not related to BL. Both BL and EL have usually chroma location top_left and it seems that 0.5px luma to the top is causing the shift.

erazortt commented 1 year ago

You won't happen to have a short clip of that video so I can give it a test?

Asd-g commented 1 year ago

The clip for images 1, 2, 3, 4 and the used scripts:

bl=FFVideoSource("BL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("EL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "RPU.bin")
Subtitle("BL-444, EL-444", size=48)
bl=FFVideoSource("BL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("EL.mkv").z_ConvertFormat(pixel_type="yuv420p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "RPU.bin")
Subtitle("BL-444, EL-420", size=48)
bl=FFVideoSource("BL.mkv").z_ConvertFormat(pixel_type="yuv420p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("EL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "RPU.bin")
Subtitle("BL-420, EL-444", size=48)
bl=FFVideoSource("BL.mkv").z_ConvertFormat(pixel_type="yuv420p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("EL.mkv").z_ConvertFormat(pixel_type="yuv420p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "RPU.bin")
Subtitle("BL-420, EL-420", size=48)

The clip for images 5, 6. Here the used source for chroma testing and the scripts:

bl=FFVideoSource("D:\FEL_test\BL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("D:\FEL_test\EL.mkv").z_ConvertFormat(pixel_type="yuv420p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "D:\FEL_test\RPU.bin")
extractu()
Subtitle("Plane U of EL-420 1080 scaled to 2160 444", size=48)
bl=FFVideoSource("D:\FEL_test\BL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
el=FFVideoSource("D:\FEL_test\EL.mkv").z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left")
DoviBaker(bl, el, "D:\FEL_test\RPU.bin")
extractu()
Subtitle("Plane U of EL-444 1080 scaled to 2160 444", size=48)
erazortt commented 1 year ago

Ok great. But I guess I'd also need the RPU.bin file. You can just give me the whole RPU file, and tell me to which frame I need to fast forward to.

Asd-g commented 1 year ago

I didn't provide them because they could be get from EL.mkv. RPU files. Frame 0 for the first clip (images 1, 2, 3, 4), frame 18 for the second clip (images 5, 6).

erazortt commented 1 year ago

Yup, I agree, there is a vertical chroma shift when the clips are subsampled. I however currently do not see why that is the case...

Asd-g commented 1 year ago

Added parameter outYUV to omit any internal colorspace/subsampling conversion when cube files are not used. A bit cleaning and two fixes.

Asd-g commented 1 year ago

Also,

pixel_type="yuv444p16", chromaloc_op="top_left=>top_left"

This really does not make sense 444 does not have a chroma siting.

444 is the destination but the source is 420 with chroma location top_left.

erazortt commented 1 year ago

I found the issue of the chroma shift. Its introduced by z_ConvertFormat!! As proof try this script:

FFVideoSource("BL.mkv")
Interleave(PointResize(Width()*2,Height()*2).UToY().ConvertToY(), z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left").UToY().ConvertToY())

Look at the pixels of distinct features directly on the left edge. You will see that the frame is moved to the left and to the top!

To fix this use chromaloc_op="center=>top_left" instead!

I really think that z_ConvertFormat should have had a "none" chromloc position to being able to sensibly use the chromaloc_op in this case as: chromaloc_op="none=>top_left". This would make much more sense!

UPDATE: I was wrong. Forget all that is written in this comment. There is some issue with the chroma.

erazortt commented 1 year ago

Now that the chromashift issue is solved, I will continue with my thoughts on your actual pull request, namely the crash when the subsampling of bl and el are different. Is this really relevant? The specifications state that: Locations of chroma samples relative to corresponding luma samples shall be the same for BL picture and EL picture.

So a clip where bl and el have different subsampling is not following the specs. And so how should a user ever be confronted with this situation? How did you arrive at different subsamplings?

Asd-g commented 1 year ago

I found the issue of the chroma shift. Its introduced by z_ConvertFormat!! As proof try this script:

FFVideoSource("BL.mkv") Interleave(PointResize(Width()2,Height()2).UToY().ConvertToY(), z_ConvertFormat(pixel_type="yuv444p16", chromaloc_op="top_left=>top_left").UToY().ConvertToY())

Look at the pixels of distinct features directly on the left edge. You will see that the frame is moved to the left and to the top!

To fix this use chromaloc_op="center=>top_left" instead!

BL has chroma location top left (check frame props) not center. When converting to 444 you need to account the vertical offset and horizontal offset.

The correct test is:

FFVideoSource("BL.mkv")
z_ConvertFormat(pixel_type="rgbp", colorspace_op="2020=>rgb", chromaloc_op="top_left=>top_left") # top_left because BL has frame prop top_left not center
bl=FFVideoSource("BL.mkv").ConvertBits(16)
el=FFVideoSource("EL.mkv").ConvertBits(16)
DoViBaker(bl, el, "RPU.bin")

Use the clip with that red light because is more obvious and compare the results from above scripts.

Is this really relevant? The specifications state that: Locations of chroma samples relative to corresponding luma samples shall be the same for BL picture and EL picture.

So a clip where bl and el have different subsampling is not following the specs. And so how should a user ever be confronted with this situation? How did you arrive at different subsamplings?

I just played with the filter. Also saw this commit 25306831b663f79278f84b054fea3f068a18b29d and when tried with BL and EL with different subsampling (they are allowed by the filter) there was crash.

erazortt commented 1 year ago

Yeah. you're tight there is something wrong with the chroma. But how sure are we that z_ConvertFormat does the right thing?

erazortt commented 1 year ago

So the thing with the chroma shift is apparently the upsampling procedures which I got from the paper ETSI GS CCM 001 V1.1.1. These apparently assume the center-left chroma location. So until further notice, I would suggest to only use this plugin with yuv444 sources. Thus yuv420 sources should be first scaled by: z_ConvertFormat(pixel_type="YUV444P16", chromaloc_op="top_left=>center")

Asd-g commented 1 year ago

Yeah. you're tight there is something wrong with the chroma. But how sure are we that z_ConvertFormat does the right thing?

z_ConvertFormat is ok. You can use fmtconv if you want:

FFVideoSource("BL.mkv")
fmtc_resample (css="444", cplace="top_left")
fmtc_matrix (mat="2020", col_fam="RGB")
fmtc_bitdepth (bits=8)

This uses ffmpeg, are you sure it correctly does chromaloc_op? Because ffmpeg cannot do it due to a bug. You need to actually force the convertion to RGB or to 444.

chromaloc_op (z_ConvertFormat) is part of zimg lib not ffmpeg. ffmpeg is used only to load the video. Any other further conversion has nothing related to ffmpeg.

Destination does not have chroma location (or chroma siting, as it is usually called).

The syntax of z_ConvertFormat for chroma location is: z_ConvertFormat(chromaloc_op="src=>dst"). 444 doesn't have chroma location but the z_ConvertFormat syntax requires some location to be used. For 420->444 only src matters and dst can be set to center, top_left, bottom... without doing anything.

erazortt commented 1 year ago

Ok. I think I corrected the chroma shift bug in v0.1.3. Thanks a lot Asd-g for pointing it out!

Asd-g commented 1 year ago

Thanks for the update. No chroma shift with v0.1.3.

Will you remove the possibility BL and EL have different subsampling?

Would you add option like outYUV to output YUV instead RGB in cases when cube files are not used? Now the scenario when one doesn't use cube files is - yuv_input->dovibaker_rgb->convertback_to_yuv in order to encode the video. The conversion to RGB and from RGB is completely unnecessary.

erazortt commented 1 year ago

I will leave the possibility for different subsamplings. When 4K is too much and the user settles for FullHD, he can downsize either the whole BL or only the Luma of BL (yielding 444). Due to the following conversion to RGB it does make sense to not downscale the Chroma planes in that step, since then they need to be again upscaled for the RGB conversion.

The thing with outYUV is that the cube files are not the only reason for the RGB conversion! DolbyVision does also provide the RGB matrix elements, thus it can introduce a non-standard conversion frame-by-frame, with no possibility regain this color changes at a later step outside of this plugin. This is actually one of the reasons I felt compelled to write this plugin in the first place. So this is why I am reluctant to incorporate this possibility, because it will leave out an important step of the whole DolbyVision processing workflow. Take a look here into the rgb conversion function, and you will see that all coefficients are from the DolbyVision stream: https://github.com/erazortt/DoViBaker/blob/27e9beb599dc5f5c5271a038a598127adf2ee326/include/DoViProcessor.h#L250 The coefficients are read from the stream and saved into these variables here: https://github.com/erazortt/DoViBaker/blob/27e9beb599dc5f5c5271a038a598127adf2ee326/DoViBaker/DoViProcessor.cpp#L217

erazortt commented 1 year ago

I am talking about one of the few freely accessible papers describing DolbyVision: ETSI GS CCM 001 V1.1.1 The upscaling algorithm is spelled out there in Annex B. But that leads to the chroma shift. Thus I first tried to figure out what algorithms these actually were that the paper discribed. And then reimplemented those in the same spirit but such that the luma and chroma planes are handeled correctly. This is how I ended up using the current implementation.

erazortt commented 1 year ago

Concerning the comparison to reference implementations. I would be very grateful if you could name some.

erazortt commented 1 year ago

I also read through the patents, but these are not much of a help regarding the resampling.

Asd-g commented 1 year ago

The thing with outYUV is that the cube files are not the only reason for the RGB conversion! DolbyVision does also provide the RGB matrix elements, thus it can introduce a non-standard conversion frame-by-frame, with no possibility regain this color changes at a later step outside of this plugin. This is actually one of the reasons I felt compelled to write this plugin in the first place. So this is why I am reluctant to incorporate this possibility, because it will leave out an important step of the whole DolbyVision processing workflow. Take a look here into the rgb conversion function, and you will see that all coefficients are from the DolbyVision stream: https://github.com/erazortt/DoViBaker/blob/27e9beb599dc5f5c5271a038a598127adf2ee326/include/DoViProcessor.h#L250 The coefficients are read from the stream and saved into these variables here: https://github.com/erazortt/DoViBaker/blob/27e9beb599dc5f5c5271a038a598127adf2ee326/DoViBaker/DoViProcessor.cpp#L217

Do you have sample where you can see difference between:

...
DoviBaker(bl, el, "RPU.bin")
...
DoviBaker(bl, el, "RPU.bin", outyuv=true)
z_ConvertFormat(pixel_type="rgbp16", colorspace_op="2020=>rgb", chromaloc_op="top_left=>top_left")
erazortt commented 1 year ago

For anybody interested, especially @Asd-g since you asked. You can analyse the RPU file for non-standard RGB conversions using the executable DoViAnalyzer.exe (which is included). The output will be something like that:

clip length: 196092 overall max cll: 991 color matrix deviation: 0 mapping deviation: 0 el-clip processing: disabled

If the two entries "color matrix deviation" and "mapping deviation" are showing a 0 like in the example, then the conversion to RGB is following the standard and the output should be identical to when using outYUV=true. However when any of these entries are different from 0, then the the RGB conversion is non-standard. From the handfull of movies I tested about half show a non-standard RGB conversion.

Asd-g commented 1 year ago

Thanks for the info.

Can you share image with outyuv=true and outyuv=false (where the difference is visible) from movie that has any of color matrix deviation / mapping deviation different than 0? I have a short clip with mapping deviation: 127 but cannot see difference between outyuv=true/false.