1337589 - gfxUtils::Get3x3YuvColorMatrix shouldn't const_cast

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1337589 - Improve Get*YuvColorMatrix. -

Review commit: https://reviewboard.mozilla.org/r/110552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110552/

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8834714 [details]
Bug 1337589 - Improve Get*YuvColorMatrix. -

https://reviewboard.mozilla.org/r/110552/#review111826

::: gfx/gl/GLBlitHelper.cpp:768
(Diff revision 1)
>      if (needsAllocation) {
>          mGL->fUniform2f(mYTexScaleLoc, (float)yuvData->mYSize.width/yuvData->mYStride, 1.0f);
>          mGL->fUniform2f(mCbCrTexScaleLoc, (float)yuvData->mCbCrSize.width/yuvData->mCbCrStride, 1.0f);
>      }
>  
> -    float* yuvToRgb = gfxUtils::Get3x3YuvColorMatrix(yuvData->mYUVColorSpace);
> +    const auto& yuvToRgb = gfxUtils::YuvToRgbMatrix3x3ColumnMajor(yuvData->mYUVColorSpace);

Please make these 'const float\*', saving a char with auto isn't worth it in this case imo.

Same with the below uses of auto.

::: gfx/thebes/gfxUtils.cpp:1200
(Diff revision 1)
> +  switch (aYUVColorSpace) {
> +  case YUVColorSpace::BT601:
> +    return rec601;
> +  case YUVColorSpace::BT709:
> +    return rec709;
> +  default:

You could make this case YUVColorSpace::UNKNOWN:, so that adding a new type to the enum (however unlile that is) would become a compile error.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8834714 [details]
Bug 1337589 - Improve Get*YuvColorMatrix. -

https://reviewboard.mozilla.org/r/110552/#review111826

> You could make this case YUVColorSpace::UNKNOWN:, so that adding a new type to the enum (however unlile that is) would become a compile error.

*unlikely.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8834714 [details]
Bug 1337589 - Improve Get*YuvColorMatrix. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110552/diff/1-2/

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8834714 [details]
> Bug 1337589 - Improve Get*YuvColorMatrix. -
> 
> https://reviewboard.mozilla.org/r/110552/#review111826
> 
> ::: gfx/gl/GLBlitHelper.cpp:768
> (Diff revision 1)
> >      if (needsAllocation) {
> >          mGL->fUniform2f(mYTexScaleLoc, (float)yuvData->mYSize.width/yuvData->mYStride, 1.0f);
> >          mGL->fUniform2f(mCbCrTexScaleLoc, (float)yuvData->mCbCrSize.width/yuvData->mCbCrStride, 1.0f);
> >      }
> >  
> > -    float* yuvToRgb = gfxUtils::Get3x3YuvColorMatrix(yuvData->mYUVColorSpace);
> > +    const auto& yuvToRgb = gfxUtils::YuvToRgbMatrix3x3ColumnMajor(yuvData->mYUVColorSpace);
> 
> Please make these 'const float\*', saving a char with auto isn't worth it in
> this case imo.
> 
> Same with the below uses of auto.
Ok, but saving chars is not why I use const auto&.

> 
> ::: gfx/thebes/gfxUtils.cpp:1200
> (Diff revision 1)
> > +  switch (aYUVColorSpace) {
> > +  case YUVColorSpace::BT601:
> > +    return rec601;
> > +  case YUVColorSpace::BT709:
> > +    return rec709;
> > +  default:
> 
> You could make this case YUVColorSpace::UNKNOWN:, so that adding a new type
> to the enum (however unlile that is) would become a compile error.

This doesn't work on MSVC:
Warning: C4715 in c:\dev\mozilla\gecko-cinn2\gfx\thebes\gfxutils.cpp: 'gfxUtils::YuvToRgbMatrix3x3ColumnMajor': not all control paths return a value

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8834714 [details]
Bug 1337589 - Improve Get*YuvColorMatrix. -

https://reviewboard.mozilla.org/r/110552/#review111826

> Please make these 'const float\*', saving a char with auto isn't worth it in this case imo.
> 
> Same with the below uses of auto.

`auto` isn't for saving chars here. It's a safe way to routinely imply the type while keeping lifetimes safe.
gfx/layers isn't my code, so don't care there. Reverted.

> *unlikely.

This doesn't work in MSVC.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8834714 [details]
Bug 1337589 - Improve Get*YuvColorMatrix. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110552/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0649459cb127
Improve Get*YuvColorMatrix. - r=mattwoodrow

Comment 9

[Robert Longson [:longsonr]]

You missed one of the const auto reversions. One crept into the patch you just landed in gfx/gl/GLBlitHelper.cpp FWIW.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0649459cb127