1457546 - CompositorOGL profiler screenshot mechanism crashes on my Android phone (mGL->fMapBuffer is null?)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[Markus Stange [:mstange]]

https://crash-stats.mozilla.com/report/index/1c82744a-bf47-48be-9914-29c110180427

I just tried out the profiler screenshot functionality on my Android phone for the first time and crashed instantly.

It's crashing in this line:

  uint8_t* srcData = static_cast<uint8_t*>(
    mGL->fMapBuffer(LOCAL_GL_PIXEL_PACK_BUFFER, LOCAL_GL_READ_ONLY));

It looks like glMapBuffer is not part of GLES 2.0 and we need to use the OES_mapbuffer extension instead.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1457546 - Add support for GL_OES_mapbuffer.

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

Comment 2

[Markus Stange [:mstange]]

This patch fixes the crash for me, but the screenshots are now black.

I'm unsure about the values that I put into the list in GLContextFeatures.cpp. Is there a convenient list online where I can look up the correct values?

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8971754 [details]
Bug 1457546 - Add support for GL_OES_mapbuffer.

https://reviewboard.mozilla.org/r/240526/#review246572

::: gfx/gl/GLContextFeatures.cpp:372
(Diff revision 1)
>          }
>      },
>      {
> +        "mapbuffer",
> +        GLVersion::GL2,
> +        GLESVersion::ES2,

ES3

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Markus Stange [:mstange] from comment #0)
> https://crash-stats.mozilla.com/report/index/1c82744a-bf47-48be-9914-
> 29c110180427
> 
> I just tried out the profiler screenshot functionality on my Android phone
> for the first time and crashed instantly.
> 
> It's crashing in this line:
> 
>   uint8_t* srcData = static_cast<uint8_t*>(
>     mGL->fMapBuffer(LOCAL_GL_PIXEL_PACK_BUFFER, LOCAL_GL_READ_ONLY));
> 
> It looks like glMapBuffer is not part of GLES 2.0 and we need to use the
> OES_mapbuffer extension instead.

OES_mapbuffer only supports WRITE_ONLY:
https://www.khronos.org/registry/OpenGL/extensions/OES/OES_mapbuffer.txt

Comment 5

[Kelsey Gilbert [:jgilbert]]

You probably want:
https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_map_buffer_range.txt

Comment 6

[Markus Stange [:mstange]]

I see! Thanks. That probably also explains the black screenshots.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Why isn't there a glReadPixels fallback?

Comment 8

[Markus Stange [:mstange]]

Because I didn't know it was needed. And I'd like this profiler feature to have as little overhead as possible.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Oh, you 100% need it on older Android devices. Many of these won't even support PBOs.

Comment 10

[Markus Stange [:mstange]]

Is there any other way to do async readback on those devices?

Comment 11

[Kelsey Gilbert [:jgilbert]]

No, ES2 core provides no async anything.

Comment 12

[Markus Stange [:mstange]]

Ok, thanks!

Comment 13

[Kelsey Gilbert [:jgilbert]]

Just to clarify, the only 'async' here is that we try to wait until the next frame to pick up the data, right?

As long as this is exceptional, this is fine. If we're expecting this to have minimal performance impact, we need to do better though.

Comment 14

[Markus Stange [:mstange]]

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Just to clarify, the only 'async' here is that we try to wait until the next
> frame to pick up the data, right?

That's right. Specifically, we wait until after the SwapBuffers() of the next frame has completed.

> As long as this is exceptional, this is fine.

It's exceptional in the sense that we'll only do it while the profiler is running with the "Screenshots" checkbox checked.
It's less exceptional in the sense that we capture every single composite when we're in this mode.

> If we're expecting this to have minimal performance impact, we need to do better though.

Oh, how?

Comment 15

[Kelsey Gilbert [:jgilbert]]

We need to inject FenceSyncs and poll on them, only mapping after we've passed the driver-side Fence. If you map a buffer that still has changes pending, it'll stall the pipeline to wait for the updates to be complete and visible to the API.

I wrote about this mechanism for webgl2 here:
https://jdashg.github.io/misc/async-gpu-downloads.html

Comment 16

[Kelsey Gilbert [:jgilbert]]

Specifically, waiting until after SwapBuffers (of the next frame? So two frames of latency?) 'should' be enough that the raw rendering has completed, but there's a little bit of question-mark for whether that data is migrated to immediately-mappable yet.

With two frames of latency, it's prooobably fine, though fencing and waiting anyways (even if just at frame boundaries) is what I'd recommend to better show intent to the driver. (it's The Right Thing To Do)

Comment 17

[Markus Stange [:mstange]]

Thank you very much! I'll give that a shot when I get a chance.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 20

[Barret Rennie [:barret] (they/them)]

Attachment: Bug 1457546 - Add checkbox to record screenshots in new performance pane r=gregtatum

Comment 21

[Barret Rennie [:barret] (they/them)]

Attachment: Bug 1457546 - Add support for GL_OES_mapbuffer. r?jgilbert

MozReview-Commit-ID: 9Kk6ylDtFti

Depends on D12339

Comment 22

[Barret Rennie [:barret] (they/them)]

Attachment: Bug 1457546 - Use glMapBufferRange instead of glMapBuffer to capture screenshots r=jgilbert

It was pointed out in a review by jgilbert that glMapBuffer only supports
writing to the mapped range and using it to read is undefined behaviour. We now
use glMapBufferRange, which does support reading, and allows capturing
screenshots on Android.

Depends on D12340

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a14f9ec04145
Add checkbox to record screenshots in new performance pane r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/99433995b7cd
Use glMapBufferRange instead of glMapBuffer to capture screenshots r=jgilbert

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a14f9ec04145
https://hg.mozilla.org/mozilla-central/rev/99433995b7cd