Fix gpu mipmap support on ChromeOS

Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/0c614148f9336a81f3cdbc2da692528811f15fc8
Cr-Commit-Position: refs/heads/master@{#406924}

[Justin Novosad, 2016-07-19 18:10:27]

Description was changed from

==========
Fix gpu mipmap support on ChromeOS

OpenGL ES requires external and internal texture formats to be set to
the same value with glTexImage2D. Therefore, GL_BGRA must be supported
as an internal format if it is to be used as an external format.
Some desktop GL implementation do not fully support the use of GL_BGRA
as an internal format. For example Mesa does not support mipmapping
with GL_BGRA textures. On desktop GL, GL_RGBA and GL_BGRA can be used
interchangeably as internal formats.  So as a workaround, we make the
GLES implementation replace BGRA with RGBA when targeting a desktop
gl backend.

BUG=540761
==========

to

==========
Fix gpu mipmap support on ChromeOS

OpenGL ES requires external and internal texture formats to be set to
the same value with glTexImage2D. Therefore, GL_BGRA must be supported
as an internal format if it is to be used as an external format.
Some desktop GL implementation do not fully support the use of GL_BGRA
as an internal format. For example Mesa does not support mipmapping
with GL_BGRA textures. On desktop GL, GL_RGBA and GL_BGRA can be used
interchangeably as internal formats.  So as a workaround, we make the
GLES implementation replace BGRA with RGBA when targeting a desktop
gl backend.

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-07-19 18:11:15]

The CQ bit was checked by junov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 18:11:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 18:11:53]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Justin Novosad, 2016-07-19 18:36:25]

junov@chromium.org changed reviewers:
+ zmo@chromium.org

[Justin Novosad, 2016-07-19 18:36:26]

PTAL

[Justin Novosad, 2016-07-19 19:15:50]

The CQ bit was unchecked by junov@chromium.org

[Zhenyao Mo, 2016-07-19 19:49:42]

zmo@chromium.org changed reviewers:
+ piman@chromium.org

[Zhenyao Mo, 2016-07-19 19:49:42]

Looks good to me, but I don't know much about the BGRA usage on desktop in
chrome.

piman: can you sign off on this?

[piman, 2016-07-19 20:32:31]

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.cc:2890: return GL_RGBA8;
This transformation should already happen at a lower level - see
GetTexInternalFormat/GetInternalFormat in ui/gl/gl_gl_api_implementation.cc.
What is it that requires this?

(in the future, I want to coalesce all this, see also crbug.com/628064, but for
now I'd prefer keeping these things in a single place, otherwise it gets super
confusing).

[Justin Novosad, 2016-07-19 21:45:26]

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.cc:2890: return GL_RGBA8;
Thanks for pointing this out. I did a bit of investigation and I found that on
ChromeOS (ozone) the gl implementation gets set to kGLImplementationEGLGLES2,
which prevents the substitution from happening in GetInternalFormat

I don't know enough about the ChromeOS config, but could this be a
misrepresentation of the Mesa driver?  make an exception for Mesa?

[piman, 2016-07-19 21:56:09]

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2164723003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.cc:2890: return GL_RGBA8;
On 2016/07/19 21:45:25, Justin Novosad wrote:
> Thanks for pointing this out. I did a bit of investigation and I found that on
> ChromeOS (ozone) the gl implementation gets set to kGLImplementationEGLGLES2,
> which prevents the substitution from happening in GetInternalFormat
> 
> I don't know enough about the ChromeOS config, but could this be a
> misrepresentation of the Mesa driver?  make an exception for Mesa?

Ah, Ozone indeed does use the ES backend of Mesa. This substitution would be
illegal on ES2 + GL_EXT_texture_format_BGRA8888.

If Mesa expects RGBA instead of BGRA as the internal format, it sounds like a
driver bug? If so we would need to add a workaround entry, and only do this
transformation if the workaround is active.

[Justin Novosad, 2016-07-19 21:59:14]

What I am observing on ChromeOS is that according to g_gl_implementation (in
src/ui/gl/gl_implementation.cc), the implementation is
kGLImplementationEGLGLES2, but according to GLVersionInfo we are running on
desktop core profile

[Justin Novosad, 2016-07-19 22:23:58]

I am not super familiar with this area of the code, but it looks to me like the
workaround entries are not meant to be accessed on the implementation side of
the dynamic bindings, Look like I'd have to do the check back in
gpu/command_buffer territory

[piman, 2016-07-19 22:52:15]

On Tue, Jul 19, 2016 at 2:59 PM, <junov@chromium.org> wrote:

> What I am observing on ChromeOS is that according to g_gl_implementation
> (in
> src/ui/gl/gl_implementation.cc), the implementation is
> kGLImplementationEGLGLES2, but according to GLVersionInfo we are running on
> desktop core profile
>

That is extremely surprising. Which Chrome OS platform is that?
E.g. On samus (Pixel 2), about:gpu reports OpenGL ES 3.1

If GLVersionInfo says desktop core profile, that indicates a bug somewhere.
That said, it would make sense to change GetInternalFormat
in ui/gl/gl_gl_api_implementation.cc to be based on GLVersionInfo rather
than GetGLImplementation(). It might be enough to fix your issue.


> https://codereview.chromium.org/2164723003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2016-07-20 14:31:55]

That worked.  New Patch.

[Justin Novosad, 2016-07-20 14:43:31]

On 2016/07/20 14:31:55, Justin Novosad wrote:
> That worked.  New Patch.

Something I noticed: I initially tried removing the condition "if
(GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a build
that cannot even show the ChromeOS login screen.

I added logging in my local build to dump the gl version info. I noticed there
is more than one instance of the bindings (gl_gl_api_implementation).  One
instance of the bindings uses a GLES profile, this instance is used for
displaying the ChromeOS login screen.  Another instance of the bindings which is
used by blink uses a desktop GL profile. Is that normal?

[Justin Novosad, 2016-07-20 15:13:38]

Description was changed from

==========
Fix gpu mipmap support on ChromeOS

OpenGL ES requires external and internal texture formats to be set to
the same value with glTexImage2D. Therefore, GL_BGRA must be supported
as an internal format if it is to be used as an external format.
Some desktop GL implementation do not fully support the use of GL_BGRA
as an internal format. For example Mesa does not support mipmapping
with GL_BGRA textures. On desktop GL, GL_RGBA and GL_BGRA can be used
interchangeably as internal formats.  So as a workaround, we make the
GLES implementation replace BGRA with RGBA when targeting a desktop
gl backend.

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-07-20 22:28:15]

On 2016/07/20 14:43:31, Justin Novosad wrote:
> On 2016/07/20 14:31:55, Justin Novosad wrote:
> > That worked.  New Patch.
> 
> Something I noticed: I initially tried removing the condition "if
> (GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a build
> that cannot even show the ChromeOS login screen.
> 
> I added logging in my local build to dump the gl version info. I noticed there
> is more than one instance of the bindings (gl_gl_api_implementation).  One
> instance of the bindings uses a GLES profile, this instance is used for
> displaying the ChromeOS login screen.  Another instance of the bindings which
is
> used by blink uses a desktop GL profile. Is that normal?

no :((((

[piman, 2016-07-20 22:29:53]

LGTM on this. This looks like a strict improvement.

But I'm really wondering what's going on on Chrome OS.

[piman, 2016-07-20 22:36:03]

On Wed, Jul 20, 2016 at 7:43 AM, <junov@chromium.org> wrote:

> On 2016/07/20 14:31:55, Justin Novosad wrote:
> > That worked. New Patch.
>
> Something I noticed: I initially tried removing the condition "if
> (GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a
> build
> that cannot even show the ChromeOS login screen.
>
> I added logging in my local build to dump the gl version info. I noticed
> there
> is more than one instance of the bindings (gl_gl_api_implementation). One
> instance of the bindings uses a GLES profile, this instance is used for
> displaying the ChromeOS login screen. Another instance of the bindings
> which is
> used by blink uses a desktop GL profile. Is that normal?
>

+ a few folks - does this ring any bell to anyone? This seems insane as
hell - it would look like with Ozone on some device(s), the login screen
and the regular chrome would use a different GL driver - or at least the
driver would report a different version.

What is the mechanism that makes us choose ES instead of Desktop GL on
Ozone? Is it a command-line flag?


>
>
> https://codereview.chromium.org/2164723003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[marcheu1, 2016-07-20 23:28:19]

On 2016/07/20 22:36:03, piman wrote:
> On Wed, Jul 20, 2016 at 7:43 AM, <mailto:junov@chromium.org> wrote:
> 
> > On 2016/07/20 14:31:55, Justin Novosad wrote:
> > > That worked. New Patch.
> >
> > Something I noticed: I initially tried removing the condition "if
> > (GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a
> > build
> > that cannot even show the ChromeOS login screen.
> >
> > I added logging in my local build to dump the gl version info. I noticed
> > there
> > is more than one instance of the bindings (gl_gl_api_implementation). One
> > instance of the bindings uses a GLES profile, this instance is used for
> > displaying the ChromeOS login screen. Another instance of the bindings
> > which is
> > used by blink uses a desktop GL profile. Is that normal?
> >
> 
> + a few folks - does this ring any bell to anyone? This seems insane as
> hell - it would look like with Ozone on some device(s), the login screen
> and the regular chrome would use a different GL driver - or at least the
> driver would report a different version.
> 
> What is the mechanism that makes us choose ES instead of Desktop GL on
> Ozone? Is it a command-line flag?

The desktop GL drivers have not been installed on Chrome OS devices for a long
time. So from the driver side, it is impossible for us to give you desktop GL.
Are you talking about running the chrome OS build on linux?

> 
> 
> >
> >
> > https://codereview.chromium.org/2164723003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2016-07-21 14:00:42]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:00:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 14:00:55]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Justin Novosad, 2016-07-21 14:02:15]

On 2016/07/20 23:28:19, marcheu1 wrote:
> On 2016/07/20 22:36:03, piman wrote:
> > On Wed, Jul 20, 2016 at 7:43 AM, <mailto:junov@chromium.org> wrote:
> > 
> > > On 2016/07/20 14:31:55, Justin Novosad wrote:
> > > > That worked. New Patch.
> > >
> > > Something I noticed: I initially tried removing the condition "if
> > > (GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a
> > > build
> > > that cannot even show the ChromeOS login screen.
> > >
> > > I added logging in my local build to dump the gl version info. I noticed
> > > there
> > > is more than one instance of the bindings (gl_gl_api_implementation). One
> > > instance of the bindings uses a GLES profile, this instance is used for
> > > displaying the ChromeOS login screen. Another instance of the bindings
> > > which is
> > > used by blink uses a desktop GL profile. Is that normal?
> > >
> > 
> > + a few folks - does this ring any bell to anyone? This seems insane as
> > hell - it would look like with Ozone on some device(s), the login screen
> > and the regular chrome would use a different GL driver - or at least the
> > driver would report a different version.
> > 
> > What is the mechanism that makes us choose ES instead of Desktop GL on
> > Ozone? Is it a command-line flag?
> 
> The desktop GL drivers have not been installed on Chrome OS devices for a long
> time. So from the driver side, it is impossible for us to give you desktop GL.
> Are you talking about running the chrome OS build on linux?
>

I am running Chrome on a Pixel (link).  I will investigate a bit more and create
a new bug based on my findings.

[Justin Novosad, 2016-07-21 14:36:20]

The CQ bit was unchecked by junov@chromium.org

[Justin Novosad, 2016-07-21 14:38:02]

I stopped the commit. This patch does not actually fix the bug. I must have done
some kind of manipulation error yesterday.
We always have an ES profile.

[Justin Novosad, 2016-07-21 15:01:51]

The CQ bit was checked by junov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 15:02:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Novosad, 2016-07-21 15:02:15]

New Patch

[commit-bot: I haz the power, 2016-07-21 15:02:15]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Justin Novosad, 2016-07-21 15:04:29]

The CQ bit was checked by junov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 15:04:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 15:04:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[spang, 2016-07-21 15:30:16]

On Wed, Jul 20, 2016 at 6:35 PM Antoine Labour <piman@chromium.org> wrote:

>
>
> On Wed, Jul 20, 2016 at 7:43 AM, <junov@chromium.org> wrote:
>
>> On 2016/07/20 14:31:55, Justin Novosad wrote:
>> > That worked. New Patch.
>>
>> Something I noticed: I initially tried removing the condition "if
>> (GetGLImplementation() != kGLImplementationEGLGLES2)" and that gave me a
>> build
>> that cannot even show the ChromeOS login screen.
>>
>> I added logging in my local build to dump the gl version info. I noticed
>> there
>> is more than one instance of the bindings (gl_gl_api_implementation). One
>> instance of the bindings uses a GLES profile, this instance is used for
>> displaying the ChromeOS login screen. Another instance of the bindings
>> which is
>> used by blink uses a desktop GL profile. Is that normal?
>>
>
> + a few folks - does this ring any bell to anyone? This seems insane as
> hell - it would look like with Ozone on some device(s), the login screen
> and the regular chrome would use a different GL driver - or at least the
> driver would report a different version.
>
> What is the mechanism that makes us choose ES instead of Desktop GL on
> Ozone? Is it a command-line flag?
>

The freon / DRM platform code path requires use of EGL/GLES2. There isn't a
way to create a desktop GL context without X (GLX), and that code isn't
even compiled in.

Michael


>
>
> https://codereview.chromium.org/2164723003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-07-21 16:25:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 16:25:10]

Dry run: This issue passed the CQ dry run.

[piman, 2016-07-21 16:31:37]

piman@chromium.org changed reviewers:
+ marcheu@chromium.org

[piman, 2016-07-21 16:31:38]

+marcheu is this a known mesa bug?

https://codereview.chromium.org/2164723003/diff/80001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/2164723003/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:46: return GL_RGBA;
This isn't valid on ES2. Did you mean to restrict this to ES3?

[Justin Novosad, 2016-07-21 17:33:57]

https://codereview.chromium.org/2164723003/diff/80001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/2164723003/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:46: return GL_RGBA;
On 2016/07/21 16:31:37, piman wrote:
> This isn't valid on ES2. Did you mean to restrict this to ES3?

Right, because it would violate the internal == external constraint of ES2?

Done.

[Justin Novosad, 2016-07-21 17:34:13]

The CQ bit was checked by junov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 17:34:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 17:34:37]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[piman, 2016-07-21 18:14:46]

Ok, reluctant LGTM, we should really do this through a workaround, but it's hard
to wire down to that level. Also can you file a bug so that we can fix mesa?

[commit-bot: I haz the power, 2016-07-21 18:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:40:17]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-07-21 18:51:12]

Should I file the bug in Mesa's issue tracker or in ours?

[Justin Novosad, 2016-07-21 18:51:21]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:51:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 18:51:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[marcheu1, 2016-07-21 18:54:13]

On 2016/07/21 18:14:46, piman wrote:
> Ok, reluctant LGTM, we should really do this through a workaround, but it's
hard
> to wire down to that level. Also can you file a bug so that we can fix mesa?

What is the mesa bug that you're talking about? I've never heard of something
like using GL instead of GLES, the code for GL isn't compiled in, and frankly, I
don't think we should land this patch because I haven't seen a proper
explanation...

[Justin Novosad, 2016-07-21 18:55:47]

On 2016/07/21 18:54:13, marcheu1 wrote:
> On 2016/07/21 18:14:46, piman wrote:
> > Ok, reluctant LGTM, we should really do this through a workaround, but it's
> hard
> > to wire down to that level. Also can you file a bug so that we can fix mesa?
> 
> What is the mesa bug that you're talking about? I've never heard of something
> like using GL instead of GLES, the code for GL isn't compiled in, and frankly,
I
> don't think we should land this patch because I haven't seen a proper
> explanation...

No, that is not the bug, that was a red herring due to an investigation mistake
I made.
The mesa bug is that mipmapping does not work when using GL_BGRA internal
format.

[commit-bot: I haz the power, 2016-07-21 18:56:19]

Description was changed from

==========
Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-21 18:56:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-21 18:58:00]

Description was changed from

==========
Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix gpu mipmap support on ChromeOS

Fix gl version info check used to control texture internal
format substitutions so that it does the right thing on
ChromeOS.  Without this substitution we'd hit a Mesa bug
that prevents mipmapping from working when the internal
format is GL_BGRA

BUG=540761
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/0c614148f9336a81f3cdbc2da692528811f15fc8
Cr-Commit-Position: refs/heads/master@{#406924}
==========

[commit-bot: I haz the power, 2016-07-21 18:58:01]

Patchset 6 (id:??) landed as
https://crrev.com/0c614148f9336a81f3cdbc2da692528811f15fc8
Cr-Commit-Position: refs/heads/master@{#406924}

[marcheu1, 2016-07-21 19:01:12]

On 2016/07/21 18:55:47, Justin Novosad wrote:
> On 2016/07/21 18:54:13, marcheu1 wrote:
> > On 2016/07/21 18:14:46, piman wrote:
> > > Ok, reluctant LGTM, we should really do this through a workaround, but
it's
> > hard
> > > to wire down to that level. Also can you file a bug so that we can fix
mesa?
> > 
> > What is the mesa bug that you're talking about? I've never heard of
something
> > like using GL instead of GLES, the code for GL isn't compiled in, and
frankly,
> I
> > don't think we should land this patch because I haven't seen a proper
> > explanation...
> 
> No, that is not the bug, that was a red herring due to an investigation
mistake
> I made.
> The mesa bug is that mipmapping does not work when using GL_BGRA internal
> format.

Ok, I haven't dug into this at all, but for example when you say that BGRA is
required by GLES 2.0, that's not true. BGRA was added with GLES 3.0...

[Justin Novosad, 2016-07-21 19:20:00]

My understanding is that ES2 requires the internal and external format to be the
same value.
Currently, skia uses BGRA on ES2 profiles if it detects the extension
GL_EXT_texture_format_BGRA8888

[piman, 2016-07-21 19:51:58]

On Thu, Jul 21, 2016 at 12:20 PM, <junov@chromium.org> wrote:

> My understanding is that ES2 requires the internal and external format to
> be the
> same value.
>

Correct.


> Currently, skia uses BGRA on ES2 profiles if it detects the extension
> GL_EXT_texture_format_BGRA8888
>

Actually, BGRA is not even in core ES3. The only way to have BGRA on ES (2
or 3) is via  GL_EXT_texture_format_BGRA8888 or
GL_APPLE_texture_format_BGRA8888.
Mesa exposes the former. It is specified against ES2 (well, ES1 actually),
and still requires internal_format == format as is the case on unextended
ES2.
There is no specific mention of the interaction with ES3 which suggests the
same constraint is still true, as is the case with other unsized internal
formats in ES3 - see section 3.7.2 Transfer of Pixel Rectangles, table 3.3.

(this is as opposed to desktop GL where the internal format would be
GL_RGBA or GL_RGBA8).


>
>
>
>
> https://codereview.chromium.org/2164723003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.