1296731 - Tweak GetAlignedStride parameters and implementation

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Change the signature and check for overflow

GetAlignedStride template function adjusts the integer argument to be a multiple of the templated value (currently power of two).  In all but two instances, the callers take the width, multiply it by bytes per pixel, then call this function.  In a few places, the callers check if that multiplication would integer overflow, nobody checks if the adjustment to the multiple of templated value would.  In some cases we "know" the arguments are checked before, but most of the time we don't.

The proposal is to change the signature of the function to take the width and the bytes per pixel, and check for overflow.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8783026 [details] [diff] [review]
Change the signature and check for overflow

We could leave both versions of the function around, but I wanted to make sure people don't keep using it the "old way".

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8783026 [details] [diff] [review]
Change the signature and check for overflow

Review of attachment 8783026 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable to me.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1296731: Add parameter to GetAlignedStride and check for overflow.

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

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8783639 [details]
Bug 1296731: Add parameter to GetAlignedStride and check for overflow.

https://reviewboard.mozilla.org/r/73368/#review71678

::: gfx/2d/Tools.h:236
(Diff revision 1)
> -int32_t GetAlignedStride(int32_t aStride)
> +int32_t GetAlignedStride(int32_t aWidth, int32_t aBytesPerPixel)
>  {
>    static_assert(alignment > 0 && (alignment & (alignment-1)) == 0,
>                  "This implementation currently require power-of-two alignment");
>    const int32_t mask = alignment - 1;
> -  return (aStride + mask) & ~mask;
> +  CheckedInt<int32_t> stride = CheckedInt<int32_t>(aWidth) * aBytesPerPixel + mask;

Provided all the promotion rules make sense this is all good. But it may be safer/easier to make all the numbers CheckedInts just for good measure.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> > +  CheckedInt<int32_t> stride = CheckedInt<int32_t>(aWidth) * aBytesPerPixel + mask;
> 
> Provided all the promotion rules make sense this is all good. But it may be
> safer/easier to make all the numbers CheckedInts just for good measure.

Will do.  There is no CheckedInt -> int automatic conversion, so the above would fail if CheckedInt's didn't get constructed, from what I can tell, but what you suggest happens anyway, so may as well make it explicit.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8783639 [details]
Bug 1296731: Add parameter to GetAlignedStride and check for overflow.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1710e1e21df0
Add parameter to GetAlignedStride and check for overflow. r=bas

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1710e1e21df0