Closed Bug 1416540 Opened 2 years ago Closed 2 years ago

Avoid unnecessary float<->double conversions in various places

Categories

(Core :: Graphics, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Following up from bug 1416267, there appear to be a lot of places in the code where we use the double-based types (gfxMatrix, gfxSize, etc.) where instead the float-based types (gfx::Matrix, gfx::Size) will do just fine. By "do just fine" I mean that using doubles is superfluous because the values they contain come from float things, and the results they produce feed back into other floats. So in effect they are doing a round-trip from floats to doubles for no reason.

Most of this seems to just be historical - for a long time the double-based types were the only option so that's what the code used. Then as parts of code were converted to Moz2D and using floats things got mismatched and left behind and whatnot.
Attachment #8927671 - Flags: review?(matt.woodrow)
Note that these patches are just a subset of the places we do this. I noticed these in particular while doing bug 1416267, but I might file more bugs in the future with more changes like this.
Comment on attachment 8927670 [details]
Bug 1416540 - Convert a bunch of scaling code to avoid unnecessary double conversion.

https://reviewboard.mozilla.org/r/198952/#review203954


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/thebes/gfxUtils.cpp:675
(Diff revision 1)
>    if (aVal < 1.0) {
>      inverse = true;
>      aVal = 1 / aVal;
>    }
>  
> -  gfxFloat power = log(aVal)/log(kScaleResolution);
> +  float power = log(aVal)/log(kScaleResolution);

Warning: Call to 'log' promotes float to double [clang-tidy: performance-type-promotion-in-math-fn]

::: gfx/thebes/gfxUtils.cpp:675
(Diff revision 1)
>    if (aVal < 1.0) {
>      inverse = true;
>      aVal = 1 / aVal;
>    }
>  
> -  gfxFloat power = log(aVal)/log(kScaleResolution);
> +  float power = log(aVal)/log(kScaleResolution);

Warning: Call to 'log' promotes float to double [clang-tidy: performance-type-promotion-in-math-fn]

  float power = log(aVal)/log(kScaleResolution);
                          ^
                          std::log

::: gfx/thebes/gfxUtils.cpp:692
(Diff revision 1)
>    // down, or we are inverted and rounding down.
>    } else {
>      power = ceil(power);
>    }
>  
> -  gfxFloat scale = pow(kScaleResolution, power);
> +  float scale = pow(kScaleResolution, power);

Warning: Call to 'pow' promotes float to double [clang-tidy: performance-type-promotion-in-math-fn]

  float scale = pow(kScaleResolution, power);
                ^
                std::pow
Comment on attachment 8927670 [details]
Bug 1416540 - Convert a bunch of scaling code to avoid unnecessary double conversion.

https://reviewboard.mozilla.org/r/198952/#review203956
Attachment #8927670 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8927671 [details]
Bug 1416540 - Convert AnimationValue::GetStyleValue to return a float-based Size.

https://reviewboard.mozilla.org/r/198954/#review203958

::: layout/style/nsStyleTransformMatrix.cpp:1424
(Diff revision 1)
>                            &dontCareBool);
>    Matrix transform2d;
>    bool canDraw2D = transform.CanDraw2D(&transform2d);
>    if (!canDraw2D) {
> -    return gfxSize();
> +    // XXX: should this return Size(1.0f, 1.0f) instead? It's supposed to be a
> +    // scale...

Looks like this return value causes us to return true from ContainsAnimatedScale for all 3d transforms.

I guess that's likely intentional (we can't easily compute the scale factors for 3d, so just assume that they are changing?), and using (1.0f, 1.0f) would break that.

I do agree it's not the best way of handling that.

Looks like this return value would also break UpdateMinMaxScale if all the transforms were 3d, but the caller checks for canDraw2D as well.
Attachment #8927671 - Flags: review?(matt.woodrow) → review+
(In reply to Static Analysis Bot [:clangbot] from comment #7)
> C/C++ static analysis found 3 defects in this patch.
> 

Fixed by using logf and powf (although running the static analysis check locally didn't give me those errors, so I'll only get confirmation by re-pushing the updated patch).

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Looks like this return value causes us to return true from
> ContainsAnimatedScale for all 3d transforms.
> 
> I guess that's likely intentional (we can't easily compute the scale factors
> for 3d, so just assume that they are changing?), and using (1.0f, 1.0f)
> would break that.
> 
> I do agree it's not the best way of handling that.
> 
> Looks like this return value would also break UpdateMinMaxScale if all the
> transforms were 3d, but the caller checks for canDraw2D as well.

Thanks, I'll leave the code as-is then and drop my XXX comment.
Comment on attachment 8927669 [details]
Bug 1416540 - Avoid unnecessary conversion to doubles in ActiveLayerTracker code.

https://reviewboard.mozilla.org/r/198950/#review204190
Attachment #8927669 - Flags: review?(mstange) → review+
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
This is unrelated to webrender, just some cleanup of pre-existing code.
No longer blocks: stage-wr-trains
Priority: P1 → P3
Whiteboard: [wr-mvp] [gfx-noted] → [gfx-noted]
Comment on attachment 8927668 [details]
Bug 1416540 - Avoid unnecessary use of double-based types in GenerateAndPushTextMask.

https://reviewboard.mozilla.org/r/198948/#review204234
Attachment #8927668 - Flags: review?(jfkthame) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9e4ad3f029c
Avoid unnecessary use of double-based types in GenerateAndPushTextMask. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/9785c2745d8b
Avoid unnecessary conversion to doubles in ActiveLayerTracker code. r=mstange
https://hg.mozilla.org/integration/autoland/rev/c9cadd7ecfe2
Convert a bunch of scaling code to avoid unnecessary double conversion. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2466c8a4fb24
Convert AnimationValue::GetStyleValue to return a float-based Size. r=mattwoodrow
Priority: P3 → --
Won't fix for 58. Let it ride the train.
You need to log in before you can comment on or make changes to this bug.