Closed Bug 1210357 Opened 4 years ago Closed 3 years ago

VP9 colorspace BT.709 is not respected

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kagami, Assigned: sotaro)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(10 files, 24 obsolete files)

126.25 KB, video/webm
Details
116.30 KB, video/webm
Details
7.18 KB, text/x-python
Details
6.73 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.39 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
308.43 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
46.00 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
9.35 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
8.42 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
6.15 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Attached video bt601.webm
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150918092427

Steps to reproduce:

1. Create VP9 webm video in BT.601 colorspace and tag it so:
ffmpeg -loglevel verbose -f lavfi -r 1 -i testsrc=s=1280x720 -vf scale=out_color_matrix=bt601,format=yuv420p -colorspace bt470bg -c:v libvpx-vp9 -lossless 1 -t 10 -y bt601.webm

2. Create same VP9 webm video but in BT.709 colorspace and tag it so:
ffmpeg -loglevel verbose -f lavfi -r 1 -i testsrc=s=1280x720 -vf scale=out_color_matrix=bt709,format=yuv420p -colorspace bt709 -c:v libvpx-vp9 -lossless 1 -t 10 -y bt709.webm


Actual results:

Firefox always uses BT.601 colorspace for VP9 videos and thus BT.709 video is darker than it should be.


Expected results:

VP9 colorspace property should be respected, i.e. tagged BT.601 should looks like tagged BT.709. It would be also good thing to use BT.709 for non-tagged HD (>=1280x720) videos automatically since it's very probably they came from HDTV or Blu-ray which use BT.709 also.
Attached video bt709.webm
sounds more like a graphic issue to me.
Flags: needinfo?(matt.woodrow)
I think issue is caused by ignoring vpx_image colorspace property[1] in SoftwareWebMVideoDecoder.cpp on VideoData creating[2]. Since YCbCr data lacks colorspace tag, I assume you always use BT.601 coeffecients to convert it back to RGB. I'm not sure it's the only way to convert between the RGB and YCbCr in Firefox but this library[3] uses SDTV (BT.601) coeffecients only.

[1]: https://chromium.googlesource.com/webm/libvpx/+/ce3780251cd9cab3b9495fb78b7f8d2773f45acf/vpx/vpx_image.h#84
[2]: https://hg.mozilla.org/mozilla-central/file/0010c0cb259e/dom/media/webm/SoftwareWebMVideoDecoder.cpp#l217
[3]: https://hg.mozilla.org/mozilla-central/file/0010c0cb259e/gfx/ycbcr/yuv_convert.cpp#l10
See also Chromium behavior[1]. It prefers BT.709 and has height-based heuristic.

[1]: https://chromium.googlesource.com/chromium/src.git/+/7cb4525761b68e41671af9006920d568be3c5405
Yeah, that looks to be the problem.

In general our handling of different colourspaces is pretty bad.

We'll need to fix that library to support both (or find/make a replacement), and then wire it up so that we use it correctly.
Flags: needinfo?(matt.woodrow)
Component: Audio/Video: Playback → Graphics
Bug 1195152 should fix that as it will set the type based on the resolution of the image
Depends on: 1195152
Actally no, bug 1195152 is just about detecting NV12 vs YV etc..
No longer depends on: 1195152
Does this bug still reproduce?
Whiteboard: [gfx-noted]
Reproduces on Linux GL layers for me.
Hi, Vimeo is investigating VP9 and correct color conversion and rendering is something quite important in the evaluation, so it would be nice that this feature would be implemented.
Assignee: nobody → sotaro.ikeda.g
Attached patch wip patch (obsolete) — Splinter Review
Wouldn't it be possible to add support for bt2020 while at it?
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8792842 - Attachment is obsolete: true
Depends on: 1304330
(In reply to Vittorio Giovara from comment #12)
> Wouldn't it be possible to add support for bt2020 while at it?

I want to split it to a different bug. Are there test contents for it?
Summary: VP9 colorspace property is not respected → VP9 colorspace BT.709 is not respected
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> (In reply to Vittorio Giovara from comment #12)
> > Wouldn't it be possible to add support for bt2020 while at it?
> 
> I want to split it to a different bug. Are there test contents for it?

I created bug 1305510 with a sample.
Thanks!
Attached patch wip patch (obsolete) — Splinter Review
Updated YUVToARGBScale().
Attachment #8793258 - Attachment is obsolete: true
Would be nice to also handle this with DXVA surface, in particular for h264 content.. Right now it seems as if everything is handled as BT601.
See Also: → 1033124
Another thought: makes sense to assume BT.709 for VP9 by default because most new content use it and colorspace tags are rarely specified. Height-based heuristic is not reliable because people and video services tend to resize video without colorspace adjustments.

Most common case: user has 1080p BT.709 video, then resizes it to e.g. 480p and publishes. Player should use BT.709 colormatrix for that 480p video even though its height is not HD.

For VP8 the only defined colorspace in spec is BT.601 (though again, a lot of people encode BT.709 content to VP8 without adjustments... but spec is important IMO).
Blocks: 1305906
Blocks: 1305907
(In reply to Jean-Yves Avenard [:jya] from comment #18)
> Would be nice to also handle this with DXVA surface, in particular for h264
> content.. Right now it seems as if everything is handled as BT601.

Thanks for the comment, bug 1305906 is created for it.
(In reply to kagami from comment #19)
> Another thought: makes sense to assume BT.709 for VP9 by default because
> most new content use it and colorspace tags are rarely specified.
> Height-based heuristic is not reliable because people and video services
> tend to resize video without colorspace adjustments.

Thanks. Bug 1305907 is created for it.
Attached file rec601_rec709.py
Created based on attachment 8449113 [details] in Bug 1033124.
Depends on: 1306521
BasicCompositor related parts were moved to bug 1306521.
Attachment #8795182 - Attachment is obsolete: true
Attached patch patch part 2 - Change to D3D11 (obsolete) — Splinter Review
Attached patch patch part 3 - Change to D3D9 (obsolete) — Splinter Review
Attachment #8797095 - Attachment description: patch part 4 - Change to opengl → patch part 4 - Change to CompositorOGL
During creating attachment 8797343 [details] [diff] [review], I noticed that video rendering on WebGL always does not use fast path.
(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> During creating attachment 8797343 [details] [diff] [review], I noticed that
> video rendering on WebGL always does not use fast path.

Bug 1307342 is created for it.
See Also: → 1307342
Attachment #8797092 - Attachment is obsolete: true
Attached patch patch part 2 - Change to D3D11 (obsolete) — Splinter Review
Attachment #8797093 - Attachment is obsolete: true
Attachment #8797967 - Flags: review?(matt.woodrow)
Attachment #8797967 - Flags: review?(matt.woodrow) → review+
Attachment #8797095 - Attachment is obsolete: true
Attachment #8798345 - Attachment is obsolete: true
Attached patch patch part 3 - Change to D3D9 (obsolete) — Splinter Review
Attachment #8797094 - Attachment is obsolete: true
Attachment #8797343 - Flags: review?(matt.woodrow)
Attachment #8797970 - Flags: review?(matt.woodrow)
Attachment #8798347 - Flags: review?(matt.woodrow)
Attachment #8798351 - Flags: review?(matt.woodrow)
Comment on attachment 8797970 [details] [diff] [review]
patch part 2 - Change to D3D11

Review of attachment 8797970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +921,5 @@
> +
> +      if (static_cast<EffectYCbCr*>(aEffectChain.mPrimaryEffect.get())->mYUVColorSpace == YUVColorSpace::BT709) {
> +        memcpy(&mPSConstants.yuvColorMatrix, &yuv_to_rgb_rec709, sizeof(mPSConstants.yuvColorMatrix));
> +      } else {
> +        memcpy(&mPSConstants.yuvColorMatrix, &yuv_to_rgb_rec601, sizeof(mPSConstants.yuvColorMatrix));

Can you please move this all into shared code on Effect?

I think it's ok to have two versions of it (the 3x3 matrix for OGL and the 4x3 for D3d9/11), as long as they are all close together rather than in the backends.

::: gfx/layers/d3d11/CompositorD3D11.hlsl
@@ +24,5 @@
>  // z = blend op
>  // w = is premultiplied
>  uint4 iBlendConfig : register(ps, c2);
>  
> +// float3x3 did not work, then use float3x4 here.

Why did it not work?

Can we get Bas to take a look and see if this is solvable?

@@ +187,3 @@
>  float4 CalculateYCbCrColor(const float2 aTexCoords)
>  {
> +  float3 yuv;

Can you please put a comment in to replace the one that got removed? Just explaining the source of these constants mainly.

::: gfx/layers/d3d11/CompositorD3D11Shaders.h
@@ +1,4 @@
>  struct ShaderBytes { const void* mData; size_t mLength; };
> +#if 0
> +//
> +// Generated by Microsoft (R) HLSL Shader Compiler 6.3.9600.16384

This is a much older version of the shader compiler, can you try update your SDK to get the latest one?
Attachment #8797970 - Flags: review?(bas)
Comment on attachment 8798347 [details] [diff] [review]
patch part 4 - Change to CompositorOGL

Review of attachment 8798347 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +405,5 @@
>            fs << "  COLOR_PRECISION float cr = " << texture2D << "(uCbTexture, coord).a;" << endl;
>          }
>        }
>  
> +      fs << "  y = y - 0.06275;" << endl;

As with D3D11, please include a comment about where 0.06275 comes from.

@@ +955,5 @@
> +    1.16438f, 1.16438f, 1.16438f, 0.0f, -0.39176f, 2.01723f, 1.59603f, -0.81297f, 0.0f,
> +  };
> +  const float yuv_to_rgb_rec709[9] = {
> +    1.16438f, 1.16438f, 1.16438f, 0.0f, -0.21325f, 2.11240f, 1.79274f, -0.53291f, 0.0f,
> +  };

Same with D3D11, please put these constant arrays (and the comments about where they come from) on Effect and just retrieve them here
Attachment #8798347 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8798351 [details] [diff] [review]
patch part 3 - Change to D3D9

Review of attachment 8798351 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming that we can't resolve the 4x3 matrix issue (same with the d3d11 patch) and the constant arrays are moved to shared code.

::: gfx/layers/d3d9/LayerManagerD3D9Shaders.h
@@ +1,3 @@
>  #if 0
>  //
> +// Generated by Microsoft (R) HLSL Shader Compiler 6.3.9600.16384

Same here, very old shader compiler.
Attachment #8798351 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8797343 [details] [diff] [review]
patch part 5 - Change to GLBlitHelper

Review of attachment 8797343 [details] [diff] [review]:
-----------------------------------------------------------------

Same again, would be nice to have the constant arrays shared with the other places we need given.

Since it's used here, maybe Effect isn't the best place for it. Maybe gfxUtils?
Attachment #8797343 - Flags: review?(matt.woodrow)
Comment on attachment 8797970 [details] [diff] [review]
patch part 2 - Change to D3D11

Review of attachment 8797970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +921,5 @@
> +
> +      if (static_cast<EffectYCbCr*>(aEffectChain.mPrimaryEffect.get())->mYUVColorSpace == YUVColorSpace::BT709) {
> +        memcpy(&mPSConstants.yuvColorMatrix, &yuv_to_rgb_rec709, sizeof(mPSConstants.yuvColorMatrix));
> +      } else {
> +        memcpy(&mPSConstants.yuvColorMatrix, &yuv_to_rgb_rec601, sizeof(mPSConstants.yuvColorMatrix));

I will add a comment in a next patch.

::: gfx/layers/d3d11/CompositorD3D11.hlsl
@@ +24,5 @@
>  // z = blend op
>  // w = is premultiplied
>  uint4 iBlendConfig : register(ps, c2);
>  
> +// float3x3 did not work, then use float3x4 here.

The comment was not good. We could use float3x3, but we need to apply float3x4 because of packing rule.
  https://msdn.microsoft.com/en-us/library/windows/desktop/bb509632(v=vs.85).aspx

@@ +187,3 @@
>  float4 CalculateYCbCrColor(const float2 aTexCoords)
>  {
> +  float3 yuv;

I'll put the comment in a next patch.
(In reply to Sotaro Ikeda [:sotaro] from comment #40)
> 
> ::: gfx/layers/d3d11/CompositorD3D11.hlsl
> @@ +24,5 @@
> >  // z = blend op
> >  // w = is premultiplied
> >  uint4 iBlendConfig : register(ps, c2);
> >  
> > +// float3x3 did not work, then use float3x4 here.
> 
> The comment was not good. We could use float3x3, but we need to apply
> float3x4 because of packing rule.
>  

mattwoodrow, is it better to use float3x3 in this case? I used float3x4 for the consistency between the ColorMatrix data array.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8797970 [details] [diff] [review]
patch part 2 - Change to D3D11

Review of attachment 8797970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/CompositorD3D11.hlsl
@@ +24,5 @@
>  // z = blend op
>  // w = is premultiplied
>  uint4 iBlendConfig : register(ps, c2);
>  
> +// float3x3 did not work, then use float3x4 here.

Shouldn't, if we use float 3x3, that work fine, we'd just need to treat it as 3x4 when uploading the buffer.
Attachment #8797970 - Flags: review?(matt.woodrow)
Attachment #8797970 - Flags: review?(bas)
Flags: needinfo?(matt.woodrow)
Attachment #8797967 - Attachment is obsolete: true
Attachment #8799681 - Flags: review+
Attached patch patch part 2 - Change to D3D11 (obsolete) — Splinter Review
Apply the comment.
Attachment #8797970 - Attachment is obsolete: true
Attached patch patch part 3 - Change to D3D9 (obsolete) — Splinter Review
Apply the comment.
Attachment #8798351 - Attachment is obsolete: true
Attachment #8799694 - Flags: review+
Attachment #8799682 - Attachment is obsolete: true
Apply the comment.
Attachment #8798347 - Attachment is obsolete: true
Attachment #8799706 - Flags: review+
Attachment #8797343 - Attachment is obsolete: true
Attachment #8799705 - Flags: review?(matt.woodrow)
Attachment #8799692 - Flags: review?(bas)
Comment on attachment 8799705 [details] [diff] [review]
patch part 1.1 - Change to gfxUtils

Review of attachment 8799705 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUtils.h
@@ +138,5 @@
>  
>      /**
> +     * Get array of yuv to rgb conversion matrix.
> +     */
> +    static float* GetYuvColorMatrixForD3D(YUVColorSpace aYUVColorSpace);

I'd prefer Get4x3YuvColorMatrix() and Get3x3YuvColorMatrix() since that makes it size of the returned pointer more obvious.

It might even be nice to use mozilla::Array<float, 12> as the return type and get even more type safety.
Attachment #8799705 - Flags: review?(matt.woodrow) → review+
Attachment #8799692 - Flags: review?(bas) → review+
Comment on attachment 8799705 [details] [diff] [review]
patch part 1.1 - Change to gfxUtils

Review of attachment 8799705 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUtils.h
@@ +138,5 @@
>  
>      /**
> +     * Get array of yuv to rgb conversion matrix.
> +     */
> +    static float* GetYuvColorMatrixForD3D(YUVColorSpace aYUVColorSpace);

I will update the function names in a next patch. About mozilla::Array<float, 12> part, it seems better to wait bug 1309466.
Apply the comment.
Attachment #8799705 - Attachment is obsolete: true
Attachment #8800499 - Flags: review+
Attached patch patch part 2 - Change to D3D11 (obsolete) — Splinter Review
Attachment #8799692 - Attachment is obsolete: true
Attachment #8800502 - Flags: review+
Attached patch patch part 3 - Change to D3D9 (obsolete) — Splinter Review
Attachment #8799694 - Attachment is obsolete: true
Attachment #8800505 - Flags: review+
Attachment #8799706 - Attachment is obsolete: true
Attachment #8800527 - Flags: review+
Attachment #8799707 - Attachment is obsolete: true
Attachment #8800528 - Flags: review?(matt.woodrow)
Comment on attachment 8800528 [details] [diff] [review]
patch part 5 - Change to GLBlitHelper

Review of attachment 8800528 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLBlitHelper.cpp
@@ +421,5 @@
>                  GLint texCb = mGL->fGetUniformLocation(program, "uCbTexture");
>                  GLint texCr = mGL->fGetUniformLocation(program, "uCrTexture");
>                  mYTexScaleLoc = mGL->fGetUniformLocation(program, "uYTexScale");
>                  mCbCrTexScaleLoc= mGL->fGetUniformLocation(program, "uCbCrTexScale");
> +                mYuvColorMatrixLoc= mGL->fGetUniformLocation(program, "uYuvColorMatrix");

Missing space before = (and on the line before).

@@ +803,5 @@
>          mGL->fUniform2f(mYTexScaleLoc, (float)yuvData->mYSize.width/yuvData->mYStride, 1.0f);
>          mGL->fUniform2f(mCbCrTexScaleLoc, (float)yuvData->mCbCrSize.width/yuvData->mCbCrStride, 1.0f);
>      }
>  
> +    float* yuv_to_rgb = gfxUtils::Get3x3YuvColorMatrix(yuvData->mYUVColorSpace);

Local vars in this code use camel case, yuvToRgb?
Attachment #8800528 - Flags: review?(matt.woodrow) → review+
Blocks: 1305510
Update nits.
Attachment #8800502 - Attachment is obsolete: true
Attachment #8800927 - Flags: review+
Update nits.
Attachment #8800505 - Attachment is obsolete: true
Attachment #8800931 - Flags: review+
Update nits.
Attachment #8800527 - Attachment is obsolete: true
Attachment #8800937 - Flags: review+
Apply the comment.
Attachment #8800528 - Attachment is obsolete: true
Attachment #8800938 - Flags: review+
Attachment #8800938 - Attachment is obsolete: true
Attachment #8800940 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c544bfb79ca0
Handle VP9 colorspace BT.709 r=mattwoodrow,bas.schouten
https://hg.mozilla.org/mozilla-central/rev/c544bfb79ca0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/54adf091c220
Backed out changeset c544bfb79ca0 for unexpected pass of layout/reftests/ogg-video/encoded-aspect-ratio-1.html on Windows XP. r=backout a=backout
Backed out for unexpected pass of layout/reftests/ogg-video/encoded-aspect-ratio-1.html on Windows XP:

https://hg.mozilla.org/mozilla-central/rev/54adf091c220f1254b35e5ba64b9bdb482dbc942

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c544bfb79ca0da894abd38100c4379903629d28a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37721310&repo=mozilla-inbound

REFTEST TEST-UNEXPECTED-PASS | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/ogg-video/encoded-aspect-ratio-1.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/ogg-video/encoded-aspect-ratio-1-ref.html | image comparison
REFTEST TEST-UNEXPECTED-PASS | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/webm-video/encoded-aspect-ratio-1.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/webm-video/encoded-aspect-ratio-1-ref.html | image comparison
Status: RESOLVED → REOPENED
Flags: needinfo?(sotaro.ikeda.g)
Resolution: FIXED → ---
encoded-aspect-ratio-1.html was disabled Bug 623460. It seemed disabled without a clear reason.
And patches in this bug somehow made the test pass.
Flags: needinfo?(sotaro.ikeda.g)
It looks like you changed the D3D9 conversion coefficients to be more precise.

Also note that Theora (in the reftest) and VP8 only support Rec601, though AFAIK they are displayed Rec601 both before and after this patch set.
Attachment #8801975 - Flags: review?(matt.woodrow)
Attachment #8801975 - Flags: review?(matt.woodrow) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5f03141ff7
Handle VP9 colorspace BT.709 r=mattwoodrow,bas.schouten
https://hg.mozilla.org/mozilla-central/rev/2b5f03141ff7
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1310405
See Also: → 1329071
Blocks: 1337589
You need to log in before you can comment on or make changes to this bug.