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

RESOLVED FIXED in Firefox 65

Status

()

RESOLVED FIXED
11 months ago
4 months ago

People

(Reporter: mstange, Assigned: brennie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 months ago
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.
(Reporter)

Updated

11 months ago
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 2

11 months ago
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?
(Reporter)

Updated

11 months ago
Crash Signature: [@ @0x0 | mozilla::layers::AsyncReadbackBufferOGL::MapAndCopyInto]

Comment 3

11 months ago
mozreview-review
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
Attachment #8971754 - Flags: review?(jgilbert) → review+
(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
(Reporter)

Comment 6

11 months ago
I see! Thanks. That probably also explains the black screenshots.
Why isn't there a glReadPixels fallback?
(Reporter)

Comment 8

11 months ago
Because I didn't know it was needed. And I'd like this profiler feature to have as little overhead as possible.
Oh, you 100% need it on older Android devices. Many of these won't even support PBOs.
(Reporter)

Comment 10

11 months ago
Is there any other way to do async readback on those devices?
No, ES2 core provides no async anything.
(Reporter)

Comment 12

11 months ago
Ok, thanks!
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.
(Reporter)

Comment 14

11 months ago
(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?
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
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)
(Reporter)

Comment 17

11 months ago
Thank you very much! I'll give that a shot when I get a chance.
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Updated

6 months ago
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 21

4 months ago
MozReview-Commit-ID: 9Kk6ylDtFti

Depends on D12339
(Assignee)

Comment 22

4 months ago
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
Attachment #9026187 - Attachment description: Bug 1457546 - Add checkbox to record screenshots in new performance pane r?mstange → Bug 1457546 - Add checkbox to record screenshots in new performance pane r?gregtatum
Attachment #9026188 - Attachment is obsolete: true
Attachment #9026187 - Attachment description: Bug 1457546 - Add checkbox to record screenshots in new performance pane r?gregtatum → Bug 1457546 - Add checkbox to record screenshots in new performance pane r=gregtatum
(Assignee)

Updated

4 months ago
Assignee: mstange → brennie
Attachment #9026187 - Attachment description: Bug 1457546 - Add checkbox to record screenshots in new performance pane r=gregtatum → Bug 1457546 - Add checkbox to record screenshots in new performance pane r?gregtatum
Attachment #9026187 - Attachment description: Bug 1457546 - Add checkbox to record screenshots in new performance pane r?gregtatum → Bug 1457546 - Add checkbox to record screenshots in new performance pane r=gregtatum
Attachment #9026189 - Attachment description: Bug 1457546 - Use glMapBufferRange instead of glMapBuffer to capture screenshots r?jgilbert → Bug 1457546 - Use glMapBufferRange instead of glMapBuffer to capture screenshots r=jgilbert
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 23

4 months ago
Pushed by nbeleuzu@mozilla.com:
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
Keywords: checkin-needed

Comment 24

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a14f9ec04145
https://hg.mozilla.org/mozilla-central/rev/99433995b7cd
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
status-firefox61: affected → ---
You need to log in before you can comment on or make changes to this bug.