Disable unpack row length when uploading mips

Disable unpack row length when uploading mips

When uploading multiple mip levels, the current path which uses
GL_UNPACK_ROW_LENGTH doesn't work (it fails to reset this value,
and is set incorrectly for subsequent mip levels). This leads
to crashes as the buffer Skia provides to GL is not sized correctly.
This change temporarily disables the GL_UNPACK_ROW_LENGTH path when we
are uploading mips. This path was an optimization, so everything should
continue working as expected.

This is a temporary workaround until we can re-structure the code to
set GL_UNPACK_ROW_LENGTH per mip level.

BUG=609612
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1965973002

Committed: https://skia.googlesource.com/skia/+/154349b6fff27a19533ae528d43a5b52e4e11d3d

[ericrk, 2016-05-10 20:25:30]

Description was changed from

==========
Disable unpack row length when uploading mips

When uploading multiple mip levels, the current path which uses
GL_UNPACK_ROW_LENGTH doesn't work (it fails to reset this value,
and is set incorrectly for subsequent mip levels). This leads
to crashes as the buffer Skia provides to GL is not sized correctly.
This change temporarily disables the GL_UNPACK_ROW_LENGTH path when we
are uploading mips. This path was an optimization, so everything should
continue working as expected.

This is a temporary workaround until we can re-structure the code to
set GL_UNPACK_ROW_LENGTH per mip level.

BUG=609612
==========

to

==========
Disable unpack row length when uploading mips

When uploading multiple mip levels, the current path which uses
GL_UNPACK_ROW_LENGTH doesn't work (it fails to reset this value,
and is set incorrectly for subsequent mip levels). This leads
to crashes as the buffer Skia provides to GL is not sized correctly.
This change temporarily disables the GL_UNPACK_ROW_LENGTH path when we
are uploading mips. This path was an optimization, so everything should
continue working as expected.

This is a temporary workaround until we can re-structure the code to
set GL_UNPACK_ROW_LENGTH per mip level.

BUG=609612
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ericrk, 2016-05-10 20:28:07]

ericrk@chromium.org changed reviewers:
+ bsalomon@google.com, cblume@chromium.org

[bsalomon, 2016-05-10 20:31:19]

lgtm

[bsalomon, 2016-05-10 20:32:02]

Curious why this never caused any of our tests to fail.

[cblume, 2016-05-10 21:02:17]

On 2016/05/10 20:32:02, bsalomon wrote:
> Curious why this never caused any of our tests to fail.

Yeah, I am surprised as well.
Maybe our tests didn't enter that branch.
I did some quick printf debugging to see if my local machine is entering this
branch and it is not. Neither caps.unpackRowLengthSupport() nor !swFlipY are
true.

I feel silly looking at this now. It is so obviously wrong. :(
It looks like it loops from the base mip level to the smallest mip level,
setting the row length.
Since this all happens in a loop before the upload happens, each texture would
have the expected row length be the row length of the smallest mip level.

If other test machines are entering that branch, is it possible the driver is
seeing that our stride is less than the actual width when we upload the mip
level and then disregarding it? (Rather than jumping back for the stride)


I will be happy to write a proper fix. It would help immensely if I had a
machine that can repro the issue.

[ericrk, 2016-05-10 21:18:09]

On 2016/05/10 21:02:17, cblume wrote:
> On 2016/05/10 20:32:02, bsalomon wrote:
> > Curious why this never caused any of our tests to fail.
> 
> Yeah, I am surprised as well.
> Maybe our tests didn't enter that branch.
> I did some quick printf debugging to see if my local machine is entering this
> branch and it is not. Neither caps.unpackRowLengthSupport() nor !swFlipY are
> true.
> 
> I feel silly looking at this now. It is so obviously wrong. :(
> It looks like it loops from the base mip level to the smallest mip level,
> setting the row length.
> Since this all happens in a loop before the upload happens, each texture would
> have the expected row length be the row length of the smallest mip level.
> 
> If other test machines are entering that branch, is it possible the driver is
> seeing that our stride is less than the actual width when we upload the mip
> level and then disregarding it? (Rather than jumping back for the stride)
> 
> 
> I will be happy to write a proper fix. It would help immensely if I had a
> machine that can repro the issue.

We do see a scattering of crashes on Android from this bug, but most of them
showed up when I enabled GPU raster on Mac. This makes me think that most cards
(at least mobile) don't support caps.unpackRowLengthSupport(). I know that this
only repros on my mac using the discrete (non-intel) graphics.

[ericrk, 2016-05-10 21:18:14]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2016-05-10 21:18:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965973002/20001

[commit-bot: I haz the power, 2016-05-10 21:36:56]

Description was changed from

==========
Disable unpack row length when uploading mips

When uploading multiple mip levels, the current path which uses
GL_UNPACK_ROW_LENGTH doesn't work (it fails to reset this value,
and is set incorrectly for subsequent mip levels). This leads
to crashes as the buffer Skia provides to GL is not sized correctly.
This change temporarily disables the GL_UNPACK_ROW_LENGTH path when we
are uploading mips. This path was an optimization, so everything should
continue working as expected.

This is a temporary workaround until we can re-structure the code to
set GL_UNPACK_ROW_LENGTH per mip level.

BUG=609612
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Disable unpack row length when uploading mips

When uploading multiple mip levels, the current path which uses
GL_UNPACK_ROW_LENGTH doesn't work (it fails to reset this value,
and is set incorrectly for subsequent mip levels). This leads
to crashes as the buffer Skia provides to GL is not sized correctly.
This change temporarily disables the GL_UNPACK_ROW_LENGTH path when we
are uploading mips. This path was an optimization, so everything should
continue working as expected.

This is a temporary workaround until we can re-structure the code to
set GL_UNPACK_ROW_LENGTH per mip level.

BUG=609612
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/154349b6fff27a19533ae528d43a5b52e4e11d3d
==========

[commit-bot: I haz the power, 2016-05-10 21:36:57]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/154349b6fff27a19533ae528d43a5b52e4e11d3d