gfxUtils::Get3x3YuvColorMatrix shouldn't const_cast

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: gfx-noted)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 2

10 months ago
mozreview-review
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.
Attachment #8834714 - Flags: review?(matt.woodrow) → review+

Comment 3

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 5

10 months ago
(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
(Assignee)

Comment 6

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 8

10 months ago
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0649459cb127
Improve Get*YuvColorMatrix. - r=mattwoodrow
You missed one of the const auto reversions. One crept into the patch you just landed in gfx/gl/GLBlitHelper.cpp FWIW.

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0649459cb127
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.