1210357 - VP9 colorspace BT.709 is not respected

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[kagami]

Attachment: 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.

Comment 1

[kagami]

Attachment: bt709.webm

Comment 2

[Jean-Yves Avenard [:jya]]

sounds more like a graphic issue to me.

Comment 3

[kagami]

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

Comment 4

[kagami]

See also Chromium behavior[1]. It prefers BT.709 and has height-based heuristic.

[1]: https://chromium.googlesource.com/chromium/src.git/+/7cb4525761b68e41671af9006920d568be3c5405

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[Jean-Yves Avenard [:jya]]

Bug 1195152 should fix that as it will set the type based on the resolution of the image

Comment 7

[Jean-Yves Avenard [:jya]]

Actally no, bug 1195152 is just about detecting NV12 vs YV etc..

Comment 8

[u279076]

Does this bug still reproduce?

Comment 9

[Thomas Daede [:TD-Linux]]

Reproduces on Linux GL layers for me.

Comment 10

[Vittorio Giovara]

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.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 12

[Vittorio Giovara]

Wouldn't it be possible to add support for bt2020 while at it?

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 14

[Sotaro Ikeda [:sotaro]]

(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?

Comment 15

[Vittorio Giovara]

(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.

Comment 16

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Updated YUVToARGBScale().

Comment 18

[Jean-Yves Avenard [:jya]]

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.

Comment 19

[kagami]

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).

Comment 20

[Sotaro Ikeda [:sotaro]]

(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.

Comment 21

[Sotaro Ikeda [:sotaro]]

(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.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: rec601_rec709.py

Created based on attachment 8449113 [details] in Bug 1033124.

Comment 23

[Sotaro Ikeda [:sotaro]]

BasicCompositor related parts were moved to bug 1306521.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - YUV color space handling to Effect

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to D3D11

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to D3D9

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to GLBlitHelper

Comment 29

[Sotaro Ikeda [:sotaro]]

During creating attachment 8797343 [details] [diff] [review], I noticed that video rendering on WebGL always does not use fast path.

Comment 30

[Sotaro Ikeda [:sotaro]]

(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.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - YUV color space handling to Effect

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to D3D11

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to D3D9

Comment 36

[Matt Woodrow (:mattwoodrow)]

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?

Comment 37

[Matt Woodrow (:mattwoodrow)]

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

Comment 38

[Matt Woodrow (:mattwoodrow)]

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.

Comment 39

[Matt Woodrow (:mattwoodrow)]

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?

Comment 40

[Sotaro Ikeda [:sotaro]]

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.

Comment 41

[Sotaro Ikeda [:sotaro]]

(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.

Comment 42

[Bas Schouten (:bas.schouten)]

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.

Comment 43

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - YUV color space handling to Effect

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1.1 - Change to gfxUtils

Comment 45

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to D3D11

Apply the comment.

Comment 46

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to D3D9

Apply the comment.

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1.1 - Change to gfxUtils

Comment 48

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Apply the comment.

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to GLBlitHelper

Comment 50

[Matt Woodrow (:mattwoodrow)]

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.

Comment 51

[Sotaro Ikeda [:sotaro]]

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.

Comment 52

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1.1 - Change to gfxUtils

Apply the comment.

Comment 53

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to D3D11

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to D3D9

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Comment 56

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to GLBlitHelper

Comment 57

[Matt Woodrow (:mattwoodrow)]

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?

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to D3D11

Update nits.

Comment 59

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to D3D9

Update nits.

Comment 60

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to CompositorOGL

Update nits.

Comment 61

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to GLBlitHelper

Apply the comment.

Comment 62

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to GLBlitHelper

Comment 63

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1cb5d14066f&selectedJob=29146420

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c544bfb79ca0
Handle VP9 colorspace BT.709 r=mattwoodrow,bas.schouten

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c544bfb79ca0

Comment 66

[Pulsebot]

Backout by [e-mail]:
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

Comment 67

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or 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

Comment 69

[Sotaro Ikeda [:sotaro]]

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.

Comment 70

[Thomas Daede [:TD-Linux]]

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.

Comment 71

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 - Update reftest

Comment 72

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c75d533386

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5f03141ff7
Handle VP9 colorspace BT.709 r=mattwoodrow,bas.schouten

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b5f03141ff7