Issue 2318313004: emulate srgb format for generateMipmap

yizhou.jiang

Description was changed from ========== srgb generateMipmap BUG= ========== to ========== srgb generateMipmap BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ...

4 years, 3 months ago (2016-09-09 08:32:57 UTC) #1

yizhou.jiang

Description was changed from ========== srgb generateMipmap BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== srgb generateMipmap BUG= ...

4 years, 3 months ago (2016-09-09 08:35:06 UTC) #2

yizhou.jiang

yizhou.jiang@intel.com changed reviewers: + yunchao.he@intel.com

4 years, 3 months ago (2016-09-09 08:35:06 UTC) #3

yunchao

Good work, Yizhou. lgtm with nits. https://codereview.chromium.org/2318313004/diff/100001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2318313004/diff/100001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode577 gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:577: glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_encoder_fbo_); decoder_fbo_ ...

4 years, 3 months ago (2016-09-13 08:55:40 UTC) #6

yizhou.jiang

Thank you for your review, yunchao. I have revised the code, PTAL.

4 years, 3 months ago (2016-09-14 02:59:29 UTC) #7

yunchao

Yizhou, please see the comments inline and update your code. https://codereview.chromium.org/2318313004/diff/160001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/160001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6018 ...

4 years, 3 months ago (2016-09-19 15:25:48 UTC) #9

yizhou.jiang

yizhou.jiang@intel.com changed reviewers: + qiankun.miao@intel.com

4 years, 3 months ago (2016-09-20 05:33:45 UTC) #10

yunchao

One more nits. https://codereview.chromium.org/2318313004/diff/180001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/180001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6017 gpu/command_buffer/service/gles2_cmd_decoder.cc:6017: } else { Please add a ...

4 years, 3 months ago (2016-09-20 06:01:57 UTC) #11

qiankun

Great work! Some suggestions. Find a more meaningful title of this CL and add description ...

4 years, 3 months ago (2016-09-20 06:11:09 UTC) #12

yunchao

On 2016/09/20 06:11:09, qiankun wrote: > Great work! Some suggestions. > Find a more meaningful ...

4 years, 3 months ago (2016-09-20 06:17:49 UTC) #13

yizhou.jiang

Description was changed from ========== srgb generateMipmap BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== srgb generateMipmap BUG=634519 ...

4 years, 3 months ago (2016-09-20 06:20:21 UTC) #14

yizhou.jiang

Description was changed from ========== srgb generateMipmap BUG=634519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== First, do sampling ...

4 years, 3 months ago (2016-09-20 06:21:34 UTC) #15

yizhou.jiang

@yunchao @qiankun Thank you for your review, the code has been updated. PTAL.

4 years, 3 months ago (2016-09-20 06:57:28 UTC) #16

yizhou.jiang

@yunchao @qiankun Thank you for your review, I have revised the code, PTAL. https://codereview.chromium.org/2318313004/diff/160001/gpu/command_buffer/service/gles2_cmd_decoder.cc File ...

4 years, 3 months ago (2016-09-20 07:00:28 UTC) #17

yunchao

The code is much better now. Yizhou, please update the conformance test in webgl2_conformance_expectation.py.

4 years, 3 months ago (2016-09-20 07:09:32 UTC) #18

yizhou.jiang

@yunchao Thank you, I have updated webgl2_conformance_expectation.py, PTAL.

4 years, 3 months ago (2016-09-20 07:38:00 UTC) #20

yizhou.jiang

yizhou.jiang@intel.com changed reviewers: + kbr@chromium.org, piman@chromium.org, zmo@chromium.org

4 years, 3 months ago (2016-09-20 08:54:10 UTC) #21

yizhou.jiang

Description was changed from ========== First, do sampling for a srgb texture, converting it to ...

4 years, 3 months ago (2016-09-20 08:55:16 UTC) #23

yunchao

On 2016/09/20 08:54:40, yizhou.jiang wrote: > PTAL. Thanks! Yizhou's first patch in Chromium project. PTAL. ...

4 years, 3 months ago (2016-09-20 09:00:10 UTC) #24

piman

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6012 gpu/command_buffer/service/gles2_cmd_decoder.cc:6012: gl_version_info().IsAtLeastGL(4, 4)) { Is there any particular reason to ...

4 years, 3 months ago (2016-09-20 21:53:30 UTC) #25

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6012:
gl_version_info().IsAtLeastGL(4, 4)) {
Is there any particular reason to cut-off at GL 4.4? Nothing in the spec really
differs between 4.3 and 4.4 (or 4.2 or 4.5 really). Is this something that
should be treated as a bug workaround, so that we can have more flexibility
about targeting the problematic drivers?

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:337:
tex->GetLevelType(target, 0, &type, &internal_format);
Should this be base_level instead of 0?

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:338: const GLint
mipmap_level =
nit: maybe mipmap_levels, since it's a count, not an array index.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:345: GL_TEXTURE_2D,
tex->service_id(), 0);
Should this be base_level instead of 0?

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:348:
glCopyTexImage2D(GL_TEXTURE_2D, 0, internal_format, 0, 0, width, height, 0);
Do we need to make that extra copy, could we use the original texture as a
source instead? By selecting GL_NEAREST as the min filter the draw would only
use the base_level mip, so you're sure to only convert the relevant level, and
then we could restore the min filter.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:377:
glGenerateMipmapEXT(GL_TEXTURE_2D);
Why this one, is it to generate the image levels on the source texture? It seems
wasteful to force the driver to do the scaling blits, just to throw away the
data, maybe we could just create them with glTexImage2D in the loop below just
before the glFramebufferTexture2DEXT?

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:389: for (GLint level =
base_level + 1; level < mipmap_level; ++level) {
mipmap_level is computed as the number of levels including and above base_level.
For example, if base_level is 3 and width/height (for that level) are 8, then we
have:
level3 = 8x8
level4 = 4x4
level5 = 2x2
level6 = 1x1

But mipmap_level will be computed as 1+log_2(8) == 4.

So I think this loop should be:
for (GLint level = base_level + 1; level < base_level + mipmap_level; ++level)

Ken Russell (switch to Gerrit)

Thanks for this contribution Yizhou. A couple of thoughts. https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6024 gpu/command_buffer/service/gles2_cmd_decoder.cc:6024: ...

4 years, 3 months ago (2016-09-21 01:00:15 UTC) #26

Thanks for this contribution Yizhou. A couple of thoughts.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6024: } else {  // TODO If the
target is GL_TEXTURE_3D or GL_TEXTURE_2D_ARRAY,
Should be TODO(yizhao.jiang) or TODO(yizhao). Also please put comment on a new
line.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum target) {
Could you add some high-level comments for this function? If I understand
correctly, the intent here is:

- Copy the base level of the sRGB texture |tex| into a linear (non-sRGB) texture
- Perform the glGenerateMipmap call against the linear texture
- Copy each mip level of the linear texture back into the sRGB texture |tex|

Is this correct?

piman did a more thorough review of this code than I could, but I wonder if you
are already going to the trouble of iterating down the mip levels and drawing
each one, then why not just generate the mipmap directly into |tex| by, at each
level:

1) Allocate |level + 1| of |tex|
2) Bind |level + 1| of |tex| to the draw framebuffer
3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it) to LINEAR
(this should be done outside the loop)
5) Draw |level| of |tex|, implicitly into |level + 1|

That would avoid the sRGB -> linear and linear -> sRGB conversions into
textures, which are lossy. It shouldn't cause a rendering feedback loop, either,
to the best of my knowledge.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:394:
glDrawArrays(GL_TRIANGLES, 0, 6);
What's the intent of this loop? Is it intended to do a level-by-level copy of
srgb_converter_textures_[1] into "tex", with NEAREST filtering at each level?

piman

On Tue, Sep 20, 2016 at 6:00 PM, <kbr@chromium.org> wrote: > Thanks for this contribution ...

4 years, 3 months ago (2016-09-21 05:33:31 UTC) #27

On Tue, Sep 20, 2016 at 6:00 PM, <kbr@chromium.org> wrote:

> Thanks for this contribution Yizhou. A couple of thoughts.
>
>
>
> https://codereview.chromium.org/2318313004/diff/240001/
> gpu/command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/2318313004/diff/240001/
> gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6024
> gpu/command_buffer/service/gles2_cmd_decoder.cc:6024: } else { // TODO
> If the target is GL_TEXTURE_3D or GL_TEXTURE_2D_ARRAY,
> Should be TODO(yizhao.jiang) or TODO(yizhao). Also please put comment on
> a new line.
>
> https://codereview.chromium.org/2318313004/diff/240001/
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
> File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):
>
> https://codereview.chromium.org/2318313004/diff/240001/
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode327
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum
> target) {
> Could you add some high-level comments for this function? If I
> understand correctly, the intent here is:
>
> - Copy the base level of the sRGB texture |tex| into a linear (non-sRGB)
> texture
> - Perform the glGenerateMipmap call against the linear texture
> - Copy each mip level of the linear texture back into the sRGB texture
> |tex|
>
> Is this correct?
>
> piman did a more thorough review of this code than I could, but I wonder
> if you are already going to the trouble of iterating down the mip levels
> and drawing each one, then why not just generate the mipmap directly
> into |tex| by, at each level:
>
> 1) Allocate |level + 1| of |tex|
> 2) Bind |level + 1| of |tex| to the draw framebuffer
> 3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
> 4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it)
> to LINEAR (this should be done outside the loop)
> 5) Draw |level| of |tex|, implicitly into |level + 1|
>
> That would avoid the sRGB -> linear and linear -> sRGB conversions into
> textures, which are lossy. It shouldn't cause a rendering feedback loop,
> either, to the best of my knowledge.
>

I think that would be a very reasonable approach too.
Agreed that the extra conversion is lossy. OTOH, some drivers apply better
than box filters on mipmap generation. So I think without data to favor one
or the other, I would be fine with either solution.


>
> https://codereview.chromium.org/2318313004/diff/240001/
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode394
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:394:
> glDrawArrays(GL_TRIANGLES, 0, 6);
> What's the intent of this loop? Is it intended to do a level-by-level
> copy of srgb_converter_textures_[1] into "tex", with NEAREST filtering
> at each level?
>

It does a copy, but with the linear->srgb conversion, hence why it needs to
be done with a draw and not a simple copy.

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

-- 
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.

yizhou.jiang

Thanks for your review, code has been updated. Could you have another look? https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc File ...

4 years, 3 months ago (2016-09-21 08:59:23 UTC) #29

Thanks for your review, code has been updated. Could you have another look?

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6012:
gl_version_info().IsAtLeastGL(4, 4)) {
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> Is there any particular reason to cut-off at GL 4.4? Nothing in the spec
really
> differs between 4.3 and 4.4 (or 4.2 or 4.5 really). Is this something that
> should be treated as a bug workaround, so that we can have more flexibility
> about targeting the problematic drivers?

According to BlitFramebuffer and drawArrays api, when processing srgb format and
opengl version under 4.4, they also have to do decoder and encoder. when version
is over 4.4, these api work well without this workaround.
In addition, qiankun has tested generateMipmap api on Nvidia, opengl 4.5, and it
also works well without modifying the code. We suppose that mesa team is working
on a patch which would fix this problem over 4.4. So I put an version condition
here.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6024: } else {  // TODO If the
target is GL_TEXTURE_3D or GL_TEXTURE_2D_ARRAY,
On 2016/09/21 01:00:14, Ken Russell wrote:
> Should be TODO(yizhao.jiang) or TODO(yizhao). Also please put comment on a new
> line.

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:337:
tex->GetLevelType(target, 0, &type, &internal_format);
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> Should this be base_level instead of 0?

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:338: const GLint
mipmap_level =
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> nit: maybe mipmap_levels, since it's a count, not an array index.

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:345: GL_TEXTURE_2D,
tex->service_id(), 0);
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> Should this be base_level instead of 0?

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:348:
glCopyTexImage2D(GL_TEXTURE_2D, 0, internal_format, 0, 0, width, height, 0);
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> Do we need to make that extra copy, could we use the original texture as a
> source instead? By selecting GL_NEAREST as the min filter the draw would only
> use the base_level mip, so you're sure to only convert the relevant level, and
> then we could restore the min filter.

I removed copy tex to srgb_decoder_textures_[0]

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:377:
glGenerateMipmapEXT(GL_TEXTURE_2D);
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> Why this one, is it to generate the image levels on the source texture? It
seems
> wasteful to force the driver to do the scaling blits, just to throw away the
> data, maybe we could just create them with glTexImage2D in the loop below just
> before the glFramebufferTexture2DEXT?

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:389: for (GLint level =
base_level + 1; level < mipmap_level; ++level) {
On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> mipmap_level is computed as the number of levels including and above
base_level.
> For example, if base_level is 3 and width/height (for that level) are 8, then
we
> have:
> level3 = 8x8
> level4 = 4x4
> level5 = 2x2
> level6 = 1x1
> 
> But mipmap_level will be computed as 1+log_2(8) == 4.
> 
> So I think this loop should be:
> for (GLint level = base_level + 1; level < base_level + mipmap_level; ++level)
> 

Done.

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:394:
glDrawArrays(GL_TRIANGLES, 0, 6);
On 2016/09/21 01:00:14, Ken Russell wrote:
> What's the intent of this loop? Is it intended to do a level-by-level copy of
> srgb_converter_textures_[1] into "tex", with NEAREST filtering at each level?

Yes. I try to sampling converter_textures_[1] to convert to srgb format, then
draw mipmaps to the original "tex".

piman

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6012 gpu/command_buffer/service/gles2_cmd_decoder.cc:6012: gl_version_info().IsAtLeastGL(4, 4)) { On 2016/09/21 08:59:22, yizhou.jiang wrote: > ...

4 years, 3 months ago (2016-09-21 23:45:32 UTC) #30

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6012:
gl_version_info().IsAtLeastGL(4, 4)) {
On 2016/09/21 08:59:22, yizhou.jiang wrote:
> On 2016/09/20 21:53:30, piman OOO back 2016-09-12 wrote:
> > Is there any particular reason to cut-off at GL 4.4? Nothing in the spec
> really
> > differs between 4.3 and 4.4 (or 4.2 or 4.5 really). Is this something that
> > should be treated as a bug workaround, so that we can have more flexibility
> > about targeting the problematic drivers?
> 
> According to BlitFramebuffer and drawArrays api, when processing srgb format
and
> opengl version under 4.4, they also have to do decoder and encoder. when
version
> is over 4.4, these api work well without this workaround.

It seems somewhat orthogonal - the behavior of glGenerateMipmaps for sRGB (not
fully specified in any case) doesn't really mention either one of
glBlitFramebuffer or glDrawArrays. From the bug, it sounds like we're
experiencing driver differences, and those are best handled with workaround
entries, which can precisely target problematic drivers/hardware without making
the code convoluted.

I am ok if that workaround entry starts with targeting desktop GL before 4.4,
but we can refine as we test more hardware/drivers.

> In addition, qiankun has tested generateMipmap api on Nvidia, opengl 4.5, and
it
> also works well without modifying the code. We suppose that mesa team is
working
> on a patch which would fix this problem over 4.4. So I put an version
condition
> here.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:370: //
glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[0]);
nit: remove commented code.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:372:
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
Because we modify the filters of the source texture, we need to restore them at
the end of the function.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:387:
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
Why is this needed? The following code doesn't sample from the texture, so the
filters shouldn't have to be changed.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:392: for (GLint level =
base_level + 1; level < base_level + mipmap_levels; ++level) {
nit: 80 columns limit. Maybe run 'git cl format'?

Ken Russell (switch to Gerrit)

The code generally looks good to me. I agree with piman's comments that it should ...

4 years, 3 months ago (2016-09-21 23:58:38 UTC) #31

Ken Russell (switch to Gerrit)

Description was changed from ========== First, do sampling for a srgb texture, converting it to ...

4 years, 3 months ago (2016-09-21 23:59:14 UTC) #32

Ken Russell (switch to Gerrit)

Note: I edited the issue description to put it in the preferred format. (Single summary ...

4 years, 3 months ago (2016-09-21 23:59:40 UTC) #33

yizhou.jiang

Thanks for your review.Code has been updated.PTAL. https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode370 gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:370: // glBindTexture(GL_TEXTURE_2D, ...

4 years, 3 months ago (2016-09-22 03:56:51 UTC) #34

Thanks for your review.Code has been updated.PTAL.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:370: //
glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[0]);
On 2016/09/21 23:45:32, piman OOO back 2016-09-12 wrote:
> nit: remove commented code.

Done.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:372:
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
On 2016/09/21 23:45:32, piman OOO back 2016-09-12 wrote:
> Because we modify the filters of the source texture, we need to restore them
at
> the end of the function.

Done.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:372:
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
On 2016/09/21 23:45:32, piman OOO back 2016-09-12 wrote:
> Because we modify the filters of the source texture, we need to restore them
at
> the end of the function.

Done.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:387:
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
On 2016/09/21 23:45:32, piman OOO back 2016-09-12 wrote:
> Why is this needed? The following code doesn't sample from the texture, so the
> filters shouldn't have to be changed.

Done.

https://codereview.chromium.org/2318313004/diff/280001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:392: for (GLint level =
base_level + 1; level < base_level + mipmap_levels; ++level) {
On 2016/09/21 23:45:32, piman OOO back 2016-09-12 wrote:
> nit: 80 columns limit. Maybe run 'git cl format'?

Done.

piman

https://codereview.chromium.org/2318313004/diff/320001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2318313004/diff/320001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6012 gpu/command_buffer/service/gles2_cmd_decoder.cc:6012: workarounds().do_decoder_encoder_srgb_generatemipmap) { Did you mean !workarounds().do_decoder_encoder_srgb_generatemipmap ? https://codereview.chromium.org/2318313004/diff/320001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode6013 gpu/command_buffer/service/gles2_cmd_decoder.cc:6013: ...

4 years, 3 months ago (2016-09-22 19:25:00 UTC) #36

qiankun

https://codereview.chromium.org/2318313004/diff/320001/gpu/config/gpu_driver_bug_list_json.cc File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2318313004/diff/320001/gpu/config/gpu_driver_bug_list_json.cc#newcode22 gpu/config/gpu_driver_bug_list_json.cc:22: "version": "8.99", Please update the version number.

4 years, 3 months ago (2016-09-23 02:26:38 UTC) #37

yizhou.jiang

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode327 gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum target) { On 2016/09/21 01:00:14, Ken Russell wrote: ...

4 years, 3 months ago (2016-09-23 04:32:06 UTC) #38

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):

https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum target) {
On 2016/09/21 01:00:14, Ken Russell wrote:
> Could you add some high-level comments for this function? If I understand
> correctly, the intent here is:
> 
> - Copy the base level of the sRGB texture |tex| into a linear (non-sRGB)
texture
> - Perform the glGenerateMipmap call against the linear texture
> - Copy each mip level of the linear texture back into the sRGB texture |tex|
> 
> Is this correct?
> 
> piman did a more thorough review of this code than I could, but I wonder if
you
> are already going to the trouble of iterating down the mip levels and drawing
> each one, then why not just generate the mipmap directly into |tex| by, at
each
> level:
> 
> 1) Allocate |level + 1| of |tex|
> 2) Bind |level + 1| of |tex| to the draw framebuffer
> 3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
> 4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it) to
LINEAR
> (this should be done outside the loop)
> 5) Draw |level| of |tex|, implicitly into |level + 1|
> 
> That would avoid the sRGB -> linear and linear -> sRGB conversions into
> textures, which are lossy. It shouldn't cause a rendering feedback loop,
either,
> to the best of my knowledge.

hi kbr, I'm not quite sure if I understand correctly. Did you mean to attach
every mipmap levels to a draw framebuffer firstly, then using glDrawBuffers
function to define draw buffers, thus we could draw all of the mipmap levels
into the original texture only sampling once instead of level by level copying? 
And I read opengl es spec 3.0, found that in section 4.4, FRAMEBUFFER OBJECTS.
It mentioned that "If the attachment sizes are not all identical, rendering will
be limited to the largest area that can fit in all of the attachments (an
intersection of rectangles having a lower left of (0, 0) and an upper right of
(width, height) for each attachment)." So maybe we can't attach color
attachments with different sizes to a framebuffer.

yunchao

LGTM again with some suggestions. @piman and @kbr, could you take another look. https://codereview.chromium.org/2318313004/diff/340001/gpu/command_buffer/service/gles2_cmd_decoder.cc File ...

4 years, 3 months ago (2016-09-23 15:09:20 UTC) #39

yizhou.jiang

Thank's for your suggestion. I have revised the code, PTAL. @yunchao @piman @kbr https://codereview.chromium.org/2318313004/diff/320001/gpu/command_buffer/service/gles2_cmd_decoder.cc File ...

4 years, 2 months ago (2016-09-26 00:56:33 UTC) #40

piman

On 2016/09/23 04:32:06, yizhou.jiang wrote: > https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode327 > ...

4 years, 2 months ago (2016-09-26 20:10:03 UTC) #41

On 2016/09/23 04:32:06, yizhou.jiang wrote:
>
https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):
> 
>
https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum target) {
> On 2016/09/21 01:00:14, Ken Russell wrote:
> > Could you add some high-level comments for this function? If I understand
> > correctly, the intent here is:
> > 
> > - Copy the base level of the sRGB texture |tex| into a linear (non-sRGB)
> texture
> > - Perform the glGenerateMipmap call against the linear texture
> > - Copy each mip level of the linear texture back into the sRGB texture |tex|
> > 
> > Is this correct?
> > 
> > piman did a more thorough review of this code than I could, but I wonder if
> you
> > are already going to the trouble of iterating down the mip levels and
drawing
> > each one, then why not just generate the mipmap directly into |tex| by, at
> each
> > level:
> > 
> > 1) Allocate |level + 1| of |tex|
> > 2) Bind |level + 1| of |tex| to the draw framebuffer
> > 3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
> > 4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it) to
> LINEAR
> > (this should be done outside the loop)
> > 5) Draw |level| of |tex|, implicitly into |level + 1|
> > 
> > That would avoid the sRGB -> linear and linear -> sRGB conversions into
> > textures, which are lossy. It shouldn't cause a rendering feedback loop,
> either,
> > to the best of my knowledge.
> 
> hi kbr, I'm not quite sure if I understand correctly. Did you mean to attach
> every mipmap levels to a draw framebuffer firstly, then using glDrawBuffers
> function to define draw buffers, thus we could draw all of the mipmap levels
> into the original texture only sampling once instead of level by level
copying? 
> And I read opengl es spec 3.0, found that in section 4.4, FRAMEBUFFER OBJECTS.
> It mentioned that "If the attachment sizes are not all identical, rendering
will
> be limited to the largest area that can fit in all of the attachments (an
> intersection of rectangles having a lower left of (0, 0) and an upper right of
> (width, height) for each attachment)." So maybe we can't attach color
> attachments with different sizes to a framebuffer.

The suggestion is to only attach 1 level at a time, but once for every iteration
of the loop - so the FBO would only have 1 attachment at a time, and you
wouldn't run into the identical size issue.
IOW, in pseudo-code:
for (level = base; level < base + count - 1; ++level) {
  glFramebufferTexture2D(GL_FRAMEBUFFER, fbo, GL_TEXTURE_2D, texture, level +
1);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, level);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, level);
  glDrawArrays(...);
}

That way, at every step, you sample uniquely from |level|, and you render
uniquely to |level + 1|. If the filter is LINEAR, because the destination is
(roughly) half the size of the source, you get interpolation and generate a
useful mip level (if the texture is pow2, the texture is sampled exactly in the
middle of 4 samples, and you get exactly 4:1 mixing, i.e. the box filter).

I think it's useful to explore that approach, but I don't see it as blocking, so
maybe we can add a TODO/file a bug, and land this in the meantime?

LGTM

Ken Russell (switch to Gerrit)

On 2016/09/26 20:10:03, piman OOO back 2016-09-26 wrote: > On 2016/09/23 04:32:06, yizhou.jiang wrote: > ...

4 years, 2 months ago (2016-09-26 21:36:28 UTC) #42

On 2016/09/26 20:10:03, piman OOO back 2016-09-26 wrote:
> On 2016/09/23 04:32:06, yizhou.jiang wrote:
> >
>
https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right):
> > 
> >
>
https://codereview.chromium.org/2318313004/diff/240001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:327: GLenum target) {
> > On 2016/09/21 01:00:14, Ken Russell wrote:
> > > Could you add some high-level comments for this function? If I understand
> > > correctly, the intent here is:
> > > 
> > > - Copy the base level of the sRGB texture |tex| into a linear (non-sRGB)
> > texture
> > > - Perform the glGenerateMipmap call against the linear texture
> > > - Copy each mip level of the linear texture back into the sRGB texture
|tex|
> > > 
> > > Is this correct?
> > > 
> > > piman did a more thorough review of this code than I could, but I wonder
if
> > you
> > > are already going to the trouble of iterating down the mip levels and
> drawing
> > > each one, then why not just generate the mipmap directly into |tex| by, at
> > each
> > > level:
> > > 
> > > 1) Allocate |level + 1| of |tex|
> > > 2) Bind |level + 1| of |tex| to the draw framebuffer
> > > 3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
> > > 4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it) to
> > LINEAR
> > > (this should be done outside the loop)
> > > 5) Draw |level| of |tex|, implicitly into |level + 1|
> > > 
> > > That would avoid the sRGB -> linear and linear -> sRGB conversions into
> > > textures, which are lossy. It shouldn't cause a rendering feedback loop,
> > either,
> > > to the best of my knowledge.
> > 
> > hi kbr, I'm not quite sure if I understand correctly. Did you mean to attach
> > every mipmap levels to a draw framebuffer firstly, then using glDrawBuffers
> > function to define draw buffers, thus we could draw all of the mipmap levels
> > into the original texture only sampling once instead of level by level
> copying? 
> > And I read opengl es spec 3.0, found that in section 4.4, FRAMEBUFFER
OBJECTS.
> > It mentioned that "If the attachment sizes are not all identical, rendering
> will
> > be limited to the largest area that can fit in all of the attachments (an
> > intersection of rectangles having a lower left of (0, 0) and an upper right
of
> > (width, height) for each attachment)." So maybe we can't attach color
> > attachments with different sizes to a framebuffer.
> 
> The suggestion is to only attach 1 level at a time, but once for every
iteration
> of the loop - so the FBO would only have 1 attachment at a time, and you
> wouldn't run into the identical size issue.
> IOW, in pseudo-code:
> for (level = base; level < base + count - 1; ++level) {
>   glFramebufferTexture2D(GL_FRAMEBUFFER, fbo, GL_TEXTURE_2D, texture, level +
> 1);
>   glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, level);
>   glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, level);
>   glDrawArrays(...);
> }
> 
> That way, at every step, you sample uniquely from |level|, and you render
> uniquely to |level + 1|. If the filter is LINEAR, because the destination is
> (roughly) half the size of the source, you get interpolation and generate a
> useful mip level (if the texture is pow2, the texture is sampled exactly in
the
> middle of 4 samples, and you get exactly 4:1 mixing, i.e. the box filter).
> 
> 
> I think it's useful to explore that approach, but I don't see it as blocking,
so
> maybe we can add a TODO/file a bug, and land this in the meantime?
> 
> LGTM

Thanks piman@ for explaining my intent. yizhou: thanks for this contribution.
Please go ahead and land as is; I defer to piman's review.

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-27 01:12:59 UTC) #43

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 01:14:00 UTC) #44

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 01:18:08 UTC) #45

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/134914) android_compile_dbg on ...

4 years, 2 months ago (2016-09-27 01:18:09 UTC) #46

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-27 04:48:22 UTC) #47

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 04:48:40 UTC) #48

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 05:34:22 UTC) #49

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/304539)

4 years, 2 months ago (2016-09-27 05:34:23 UTC) #50

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-27 08:39:15 UTC) #51

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 08:39:31 UTC) #52

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 09:55:04 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/300290)

4 years, 2 months ago (2016-09-27 09:55:05 UTC) #54

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 01:23:42 UTC) #55

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 01:24:07 UTC) #56

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 02:28:25 UTC) #57

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/300935)

4 years, 2 months ago (2016-09-28 02:28:26 UTC) #58

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 02:55:41 UTC) #59

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 02:55:52 UTC) #60

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 03:14:21 UTC) #61

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 03:14:33 UTC) #62

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 03:37:11 UTC) #63

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_tests_rel/builds/3744)

4 years, 2 months ago (2016-09-28 03:37:12 UTC) #64

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 04:40:28 UTC) #66

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 04:40:37 UTC) #67

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 05:28:10 UTC) #68

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/301015)

4 years, 2 months ago (2016-09-28 05:28:12 UTC) #69

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 06:18:50 UTC) #72

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 06:19:00 UTC) #73

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 06:21:10 UTC) #74

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/76617) mac_chromium_rel_ng on ...

4 years, 2 months ago (2016-09-28 06:21:11 UTC) #75

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 08:33:31 UTC) #77

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 08:33:41 UTC) #78

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-28 09:32:23 UTC) #79

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/38050)

4 years, 2 months ago (2016-09-28 09:32:24 UTC) #80

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 01:04:29 UTC) #81

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 01:04:55 UTC) #82

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 02:05:58 UTC) #83

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_tests_rel/builds/4127)

4 years, 2 months ago (2016-09-29 02:05:59 UTC) #84

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 02:38:19 UTC) #85

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 02:38:37 UTC) #86

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/266221) win_chromium_rel_ng on ...

4 years, 2 months ago (2016-09-29 02:43:32 UTC) #88

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 02:48:12 UTC) #89

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 02:48:31 UTC) #90

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 02:57:53 UTC) #91

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/269236)

4 years, 2 months ago (2016-09-29 02:57:54 UTC) #92

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 05:23:27 UTC) #94

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 05:23:40 UTC) #95

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 05:33:29 UTC) #96

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/269340)

4 years, 2 months ago (2016-09-29 05:33:30 UTC) #97

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 05:58:17 UTC) #98

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 05:58:35 UTC) #99

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 06:07:21 UTC) #100

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_gpu_tests_rel/builds/2970)

4 years, 2 months ago (2016-09-29 06:07:22 UTC) #101

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 06:29:36 UTC) #102

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 06:29:56 UTC) #103

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 06:38:49 UTC) #105

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/136387)

4 years, 2 months ago (2016-09-29 06:38:50 UTC) #106

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 07:01:28 UTC) #108

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:01:51 UTC) #109

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:11:26 UTC) #110

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/269401)

4 years, 2 months ago (2016-09-29 07:11:27 UTC) #111

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 07:26:58 UTC) #112

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:27:11 UTC) #113

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:36:59 UTC) #114

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/269413)

4 years, 2 months ago (2016-09-29 07:37:00 UTC) #115

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 07:49:08 UTC) #116

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:49:31 UTC) #117

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 07:59:55 UTC) #118

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/136433)

4 years, 2 months ago (2016-09-29 07:59:56 UTC) #119

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 10:59:38 UTC) #123

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 10:59:57 UTC) #124

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-29 11:08:33 UTC) #125

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/269528)

4 years, 2 months ago (2016-09-29 11:08:35 UTC) #126

yizhou.jiang

@piman, I'm quite confused that the trybot reports "GpuProcess.equal_bug_workarounds_in_browser_and_gpu_process". There seems to be something wrong ...

4 years, 2 months ago (2016-09-29 12:03:36 UTC) #127

Zhenyao Mo

On 2016/09/29 12:03:36, yizhou.jiang wrote: > @piman, I'm quite confused that the trybot reports > ...

4 years, 2 months ago (2016-09-29 20:46:44 UTC) #128

Ken Russell (switch to Gerrit)

On 2016/09/29 20:46:44, Zhenyao Mo wrote: > On 2016/09/29 12:03:36, yizhou.jiang wrote: > > @piman, ...

4 years, 2 months ago (2016-09-29 22:24:43 UTC) #129

Julien Isorce Samsung

Hi, the failing test should work because gpu process sends the updated GpuInfo back to ...

4 years, 2 months ago (2016-09-29 23:07:26 UTC) #130

Julien Isorce Samsung

Hi, could you temporarily include the fix https://codereview.chromium.org/2379153002 in your CL and hit a CQ ...

4 years, 2 months ago (2016-09-29 23:38:17 UTC) #131

yunchao

On 2016/09/29 23:07:26, Julien Isorce wrote: > Hi, the failing test should work because gpu ...

4 years, 2 months ago (2016-09-29 23:40:15 UTC) #132

Julien Isorce Samsung

On 2016/09/29 23:40:15, yunchao wrote: > Thanks for your trying, @Julien. How to run the ...

4 years, 2 months ago (2016-09-29 23:57:51 UTC) #133

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-30 01:41:20 UTC) #137

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 01:41:44 UTC) #138

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-30 01:43:29 UTC) #139

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 01:43:45 UTC) #140

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-30 02:24:48 UTC) #141

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 02:25:12 UTC) #142

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 03:24:12 UTC) #144

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/302577)

4 years, 2 months ago (2016-09-30 03:24:13 UTC) #145

Julien Isorce Samsung

https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_bug_list_json.cc File gpu/config/gpu_driver_bug_list_json.cc (left): https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_bug_list_json.cc#oldcode2046 gpu/config/gpu_driver_bug_list_json.cc:2046: } Because of crbug.com/222934, you cannot rely on gl_version ...

4 years, 2 months ago (2016-10-04 13:41:25 UTC) #146

Zhenyao Mo

On 2016/10/04 13:41:25, Julien Isorce wrote: > https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_bug_list_json.cc > File gpu/config/gpu_driver_bug_list_json.cc (left): > > https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_bug_list_json.cc#oldcode2046 ...

4 years, 2 months ago (2016-10-04 17:34:13 UTC) #147

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-08 04:43:16 UTC) #148

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 04:43:22 UTC) #149

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-08 05:42:52 UTC) #150

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 05:43:03 UTC) #151

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 06:58:56 UTC) #152

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-08 06:58:58 UTC) #153

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-08 07:52:31 UTC) #154

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 07:52:36 UTC) #155

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-08 07:56:17 UTC) #158

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 07:56:25 UTC) #159

yizhou.jiang

The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-08 08:57:30 UTC) #162

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 08:57:40 UTC) #163

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-08 09:56:15 UTC) #164

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-08 09:56:16 UTC) #165

yizhou.jiang

On 2016/10/04 17:34:13, Zhenyao Mo wrote: > On 2016/10/04 13:41:25, Julien Isorce wrote: > > ...

4 years, 2 months ago (2016-10-08 10:10:37 UTC) #166

On 2016/10/04 17:34:13, Zhenyao Mo wrote:
> On 2016/10/04 13:41:25, Julien Isorce wrote:
> >
>
https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_...
> > File gpu/config/gpu_driver_bug_list_json.cc (left):
> > 
> >
>
https://codereview.chromium.org/2318313004/diff/750001/gpu/config/gpu_driver_...
> > gpu/config/gpu_driver_bug_list_json.cc:2046: }
> > Because of crbug.com/222934, you cannot rely on gl_version for macosx.
> > For osx you can only use os version, and vendor/device ids.
> > So you have to split your new entry into 2. First one for the
> > general case that will apply on all os except osx.
> > Second entry will be specific to macosx, see below for example:
> > 
> > {
> >   "id": 191,
> >   "description": "Decode and encode before generateMipmap for srgb format
> > textures XXX",
> >   "cr_bugs": [634519],
> >   "gl_type": "gl",
> >   "gl_version": {
> >     "op": "<",
> >     "value": "4.4"
> >   },
> >   "exceptions": [
> >     {
> >       "os": {
> >         "type": "macosx"
> >       }
> >     }
> >   ],
> >   "features": [
> >     "decode_encode_srgb_for_generatemipmap"
> >   ]
> > },
> > {
> >   "id": 192,
> >   "description": "Decode and encode before generateMipmap for srgb format
> > textures XXX",
> >   "cr_bugs": [634519],
> >   "os": {
> >     "type": "macosx",
> >     "version": {
> >       "op": "<=",
> >       "value": "M.N" // should be reasonable on osx to rely roughly gl
> >                      // version with os version
> >                      // this is what other osx entries do in this file
> >     }
> >   },
> >   // if needed, also rely on vendor and device ids
> >   "vendor_id": "X",
> >   "device_id": ["Y1", "Y2", "Y3"],
> >   "features": [
> >     "decode_encode_srgb_for_generatemipmap"
> >   ]
> > }
> 
> There is no reason to specify GL version on OSX now.  Let's just assume it's
4.1
> and move on.  If the situation ever changes, then we can revisit (I doubt it
> will go beyond 4.1)

@Julien @Zhenyao Thanks a lot for your suggestion! Code has been updated, and
trybots can pass now, PTAL, @piman @Zhenyao @kbr

yizhou.jiang

The patchset sent to the CQ was uploaded after l-g-t-m from yunchao.he@intel.com, piman@chromium.org Link to ...

4 years, 2 months ago (2016-10-11 03:01:55 UTC) #171

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-11 03:02:10 UTC) #172

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-11 03:04:47 UTC) #173

commit-bot: I haz the power

Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/84221) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...

4 years, 2 months ago (2016-10-11 03:04:50 UTC) #174

yizhou.jiang

The patchset sent to the CQ was uploaded after l-g-t-m from yunchao.he@intel.com, zmo@chromium.org, piman@chromium.org Link ...

4 years, 2 months ago (2016-10-11 05:34:42 UTC) #176

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-11 05:35:00 UTC) #177

commit-bot: I haz the power

Description was changed from ========== emulate srgb format for generateMipmap First, do sampling for a ...

4 years, 2 months ago (2016-10-11 06:21:25 UTC) #178

commit-bot: I haz the power

Description was changed from ========== emulate srgb format for generateMipmap First, do sampling for a ...

4 years, 2 months ago (2016-10-11 06:22:38 UTC) #180

commit-bot: I haz the power

4 years, 2 months ago (2016-10-11 06:22:41 UTC) #181

Message was sent while issue was closed.

Patchset 20 (id:??) landed as
https://crrev.com/da573a8ab571bb73d3539a66abbe5c2efd47d7b7
Cr-Commit-Position: refs/heads/master@{#424377}

Message was sent while issue was closed.

Patchset #21 (id:910001) has been deleted

Issue 2318313004: emulate srgb format for generateMipmap (Closed)

Description

Patch Set 1 #

Patch Set 2 : srgb-generateMipmap #

Patch Set 3 : srgb-generateMipmap #

Patch Set 4 : srgb-generateMipmap #

Patch Set 5 : srgb-generateMipmap #

Patch Set 6 : srgb-generateMipmap #

Patch Set 7 : srgb-generateMipmap #

Patch Set 8 : addressed yunchao and qiankun's feedbacks #

Patch Set 9 : addressed yunchao's feedback:update webgl2_conformance_expectation.py #

Patch Set 10 : addressed piman and kbr's feedbacks (edit) #

Patch Set 11 : addressed piman's feedback #

Patch Set 12 : addressed piman's feedback #

Patch Set 13 : addressed piman, qiankun and yunchao's feedbacks #

Patch Set 14 : addressed piman's feedback #

Patch Set 15 : trybots issue: getLevelType #

Patch Set 16 : rm feature->info #

Patch Set 17 : Test trybot issue: Applying Julien's patch 2379153002 #

Patch Set 18 : addressed feedbacks #

Patch Set 19 : Addressed to Julien and Zhenyao's feedback #

Patch Set 20 : rebase code #

Messages