Closed Bug 1337589 Opened 3 years ago Closed 3 years ago

gfxUtils::Get3x3YuvColorMatrix shouldn't const_cast

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 file)

No description provided.
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 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.
(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 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.
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.
https://hg.mozilla.org/mozilla-central/rev/0649459cb127
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.