WebGL test failure: conformance/textures/misc/tex-image-with-format-and-type

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
2 years ago
a year ago

People

(Reporter: kvark, Assigned: kvark)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

(Whiteboard: [gfx-noted], URL)

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → kvark
(Assignee)

Updated

2 years ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
(Assignee)

Comment 1

2 years ago
Created attachment 8823416 [details] [diff] [review]
webgl-tex-image.patch

Attaching a patch that fixes everything but the last test (`UNPACK_PREMULTIPLY_ALPHA_WEBGL` with `LUMINANCE_ALPHA/UNSIGNED_BYTE`).
Attachment #8823416 - Flags: review?(jgilbert)
Is this a regression?
Comment on attachment 8823416 [details] [diff] [review]
webgl-tex-image.patch

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

This is very surprising, since taking `const auto&` should bind a temporary value returned from a function, extending its lifetime to that of the reference.

What's wrong with the code that you fix here? Some unexpected type conversion/implicit coercion issue?
Flags: needinfo?(kvark)
(Assignee)

Comment 4

2 years ago
Yes, the compiler *should be* extending the lifetime of temporary references bound by `const auto&`. I didn't know that, but Botond (adding him as CC) have kindly explained this to me, but that's not the whole

> however, gcc versions earlier than 7 have a bug where this lifetime extension is not implemented for subobjects of scalar type: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54293

This affects lines like this one:
"const auto& rowLength = mSurf->GetSize().width;"
Flags: needinfo?(kvark)
Unbelievable. I thought about asking if this was a compiler bug, but figured it was unlikely. :(
From the gcc bug, it sounds like it might be only this pattern that's problematic:
const auto& fullRows = mSurf->GetSize().height

Does your fix work with only removing & from scalar-subvalues-of-temporaries?
Flags: needinfo?(kvark)
I get 26 hits in the codebase for:
git grep -n 'const auto *&.*=.*()\(.\|->\)[a-zA-Z0-9]\+'

Looks like our cases are the only problematic ones, based on my understanding and reading.
(Assignee)

Comment 8

2 years ago
Yes, I scanned the codebase as well and figured these are the only cases that apply, which is good.
(Assignee)

Comment 9

2 years ago
Created a separate bug for the gcc6 undefined behavior - https://bugzilla.mozilla.org/show_bug.cgi?id=1329044
(Assignee)

Comment 10

2 years ago
I confirm that just fixing the sub-field references addresses the relevant tests, see patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1329044
Flags: needinfo?(kvark)
(Assignee)

Updated

2 years ago
Attachment #8823416 - Attachment is obsolete: true
Attachment #8823416 - Flags: review?(jgilbert)
(Assignee)

Updated

2 years ago
Depends on: 1329044
(Assignee)

Comment 11

2 years ago
Created attachment 8825026 [details] [diff] [review]
webgl-texunpack-format.patch

Attaching the patch along the lines discussed with Jeff. The conformance test passes, but I don't know if I broke any others. I'm not super happy about the way it turns out, particularly with the "const webgl::PackingInfo pi = {0, 0}" parts.

Treeherder status:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e2eff1079e1733c70658c09abf4f9c1141ee75
Attachment #8825026 - Flags: review?(jgilbert)
(Assignee)

Updated

2 years ago
QA Contact: kvark
Whiteboard: [gfx-noted]
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
QA Contact: kvark

Comment 12

a year ago
Observed the test https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/tex-image-with-format-and-type.html?webglVersion=1&quiet=0

to fail on
 - Supermicro X10DAX 1.02 - Linux Mint 18 Sarah, Kernel 4.4.0-36-generic 64-bit, Firefox 55.0a1 (2017-05-22) (64-bit) - ASUS GeForce GTX 1060 OpenGL 3.2.0, NVIDIA 370.28
 - Apple MacBook Pro 13" 2016 - macOS Sierra 10.12.3 - Firefox Nightly 55.0a1 (2017-05-21) (64-bit) - Intel Iris Graphics 550 1536 MB 4.1 INTEL-10.22.29
 - Apple Mac Mini (Late 2012) - Ubuntu 16.04 Xenial, Kernel 4.4.0-78-generic 64-bit - Firefox 55.0a1 (2017-05-22) (64-bit) - Intel HD Graphics 4000, Intel Mesa DRI Ivybridge Mobile, OpenGL 3.3 (Core Profile) Mesa 12.0.6
 - Apple Mac Pro (Late 2013) - OS X El Capitan 10.11.6 - Firefox 55.0a1 (2017-05-22) (64-bit) - AMD FirePro D500 4.1 ATI-1.42.15
 - Apple MacBook Air 13" Mid-2011 - macOS Sierra 10.12.3 - Firefox 55.0a1 (2017-05-22) (64-bit) - Intel HD Graphics 3000 2.1 INTEL-10.2.12

but passes on
 - HP Notebook 14-am009no 14" - Windows 10 Home - Firefox 55.0a1 (2017-05-22) (64-bit) - Intel HD Graphics (Intel HD 400?) Direct3D 11, 20.19.15.4509 9-1-2016, OpenGL ES 3.0 (ANGLE 2.1.0.dec065540d5f)
 - Haswell - Windows 10 Home 64-bit - Firefox 55.0a1 (2017-05-22) (64-bit) - 2x ASUS NVidia GeForce GTX 1080 Ti Direct3D 11, v22.21.13.8189 (4-19-2017), OpenGL ES 3.0 (ANGLE 2.1.0.dec065540d5f)
 - Intel NUC6i7KYB Skull Canyon - Windows 10 Pro 10.0.14393 64-bit, Firefox 55.0a1 (2017-05-22) (64-bit) - Intel Iris Pro Graphics 580, 21.20.16.4534 (10-7-2016), OpenGL ES 3.0 (ANGLE 2.1.0.dec065540d5f)
 - Microsoft Surface Pro 2 - Windows 10 Pro, Firefox 55.0a1 (2017-05-22) (64-bit) - Intel HD Graphics 4400 Direct3D11 v20.19.15.4331 (11-18-2015), OpenGL ES 3.0 (ANGLE 2.1.0.dec065540d5f)
 - LG Nexus 5 - Android 6.0.1, Kernel 3.4.0-gcf10b7e (hammerhead M4B30Z), Fennec 55.0a1 (2017-05-19) - Qualcomm Adreno 330, OpenGL ES 3.0 V@127.9 AU@(GIT@I98aee987eb)
 - Google Pixel XL - Android 7.1.2, Kernel 3.18.31-g416bf43 (marlin N2G47O), Fennec 55.0a1 (2017-05-19) - Qualcomm Adreno 530, OpenGL ES 3.2 V@145.0 (GIT@Idb2b4cb785)
 - Samsung Galaxy S7 Edge SM-G935F - Android 7.0, Kernel 3.18.14-11104523 (NRD90M), Fennec 55.0a1 (2017-05-19), ARM Mali-T880, OpenGL ES 3.2 v1.r12p1-03dev0.228ab63cced004f840e7dd47b762a1d0
 - Huawei P10 Plus - Android 7.0, Kernel 4.1.18-gfd75bbb (VKY-L29), Fennec 55.0a1 (2017-05-19) - ARM Mali-G71, OpenGL ES 3.2 v1.r2p0-02dev0.f7269486f3e0e3b308edf85872e361f4
 - LG Nexus 4 - Android 5.1.1 Kernel 3.4.0-perf-gdffc258 (occam LMY48T), Fennec 55.0a1 (2017-05-19) - Qualcomm Adreno 320, OpenGL ES 3.0 V@104.0 AU@ (GIT@Id3510ff6dc) (no WebGL 2, missing standard_derivatives)
 - Samsung Nexus 10 - Android 5.1.1 Kernel 3.4.67-g84ad5a4 (mantaray LMY49J), Fennec 55.0a1 (2017-05-19) - ARM Mali-T604, OpenGL ES 3.1 (no WebGL2 because of lack of renderbuffer_color_float & renderbuffer_color_half_float)
 - Samsung Galaxy S4 GT-I9500 - Android 5.0.1, Kernel 3.4.5-5676501, Fennec 55.0a1 (2017-05-19) - Imagination PowerVR SGX 544MP, OpenGL ES 2.0 build 1.10@2359475 (no WebGL 2 support)

Dzmitry, it's been some time, so I wonder if the above patch of yours went in at some point, or if the failure would still be present in Nightly?
Flags: needinfo?(kvark)
(Assignee)

Comment 13

a year ago
Jukka,

As far as I'm concerned, the patch is till r? on Jeff
Jeff, are you able to review it?
Flags: needinfo?(kvark) → needinfo?(jgilbert)
We'll see where this is after bug 1372385.
Depends on: 1372385
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.