Closed Bug 826607 Opened 8 years ago Closed 5 years ago

don't block while pausing and resuming the compositor

Categories

(Firefox for Android :: Toolbar, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: blassey, Unassigned)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #697800 - Flags: review?(bugmail.mozilla)
Comment on attachment 697800 [details] [diff] [review]
patch

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

This seems sane to me. A few nits below, and I would like to see a green try run before this lands. Also adding BenWa as a reviewer.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +351,5 @@
>  
>    Composite();
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +  nsWindow::NotifyCompositorResumed();

I would rather this went through AndroidBridge rather than directly invoking nsWindow. It should also make the Makefile changes unnecessary.

::: gfx/layers/ipc/CompositorParent.h
@@ +86,5 @@
>    void ScheduleRenderOnCompositorThread();
>    void SchedulePauseOnCompositorThread();
>    void ScheduleResumeOnCompositorThread(int width, int height);
> +  void SchedulePauseOnCompositorThreadAsync();
> +  void ScheduleResumeOnCompositorThreadAsync(int width, int height);

Can we get rid of the non-Async versions? Are those called from anywhere now?
Attachment #697800 - Flags: review?(bugmail.mozilla)
Attachment #697800 - Flags: review?(bgirard)
Attachment #697800 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Comment on attachment 697800 [details] [diff] [review]
> patch
> 
> Review of attachment 697800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems sane to me. A few nits below, and I would like to see a green try
> run before this lands. Also adding BenWa as a reviewer.
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +351,5 @@
> >  
> >    Composite();
> >  
> > +#ifdef MOZ_WIDGET_ANDROID
> > +  nsWindow::NotifyCompositorResumed();
> 
> I would rather this went through AndroidBridge rather than directly invoking
> nsWindow. It should also make the Makefile changes unnecessary.

I considered that, but in the end I disagree. AndroidBridge is supposed to be a central point for interacting with Java via JNI, which this is not. Open to other suggestions though.

> ::: gfx/layers/ipc/CompositorParent.h
> @@ +86,5 @@
> >    void ScheduleRenderOnCompositorThread();
> >    void SchedulePauseOnCompositorThread();
> >    void ScheduleResumeOnCompositorThread(int width, int height);
> > +  void SchedulePauseOnCompositorThreadAsync();
> > +  void ScheduleResumeOnCompositorThreadAsync(int width, int height);
> 
> Can we get rid of the non-Async versions? Are those called from anywhere now?

Looks like we can, building with that change now.

try push: https://tbpl.mozilla.org/?tree=Try&rev=9d549ad7d400
green run for android tests here https://tbpl.mozilla.org/?tree=Try&rev=9d549ad7d400 though there was a windows build failure, so pushed an updated patch to try https://tbpl.mozilla.org/?tree=Try&rev=96d952bef2c9
It's not looking so green any more.
Try run for 96d952bef2c9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=96d952bef2c9
Results (out of 59 total builds):
    success: 41
    warnings: 17
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-96d952bef2c9
Try run for e33bbeee63e4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e33bbeee63e4
Results (out of 57 total builds):
    success: 52
    warnings: 4
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-e33bbeee63e4
Try run for 9d549ad7d400 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9d549ad7d400
Results (out of 58 total builds):
    success: 46
    warnings: 9
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-9d549ad7d400
Try run for e33bbeee63e4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e33bbeee63e4
Results (out of 60 total builds):
    success: 55
    warnings: 4
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-e33bbeee63e4
Duplicate of this bug: 803151
We made the pause and resume sync because the surface goes away when the app is hidden. If we land this patch we have to make sure that's not an issue. The problem was very timing dependent. We ran into this here:
https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c6
with more details here:
https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c12

The summary is that we have no make sure that there are no pending updates because we need a gl surface to process the updates.
(In reply to Benoit Girard (:BenWa) from comment #10)
> We made the pause and resume sync because the surface goes away when the app
> is hidden. If we land this patch we have to make sure that's not an issue.
> The problem was very timing dependent. We ran into this here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c6
> with more details here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c12
> 
> The summary is that we have no make sure that there are no pending updates
> because we need a gl surface to process the updates.

Where do we use the gl surface to process the updates?
(In reply to Brad Lassey [:blassey] from comment #11)
> (In reply to Benoit Girard (:BenWa) from comment #10)
> > We made the pause and resume sync because the surface goes away when the app
> > is hidden. If we land this patch we have to make sure that's not an issue.
> > The problem was very timing dependent. We ran into this here:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c6
> > with more details here:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c12
> > 
> > The summary is that we have no make sure that there are no pending updates
> > because we need a gl surface to process the updates.
> 
> Where do we use the gl surface to process the updates?

We need a surface for our MakeCurrent to upload to our textures:
http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#485

I believe for some time we would bind with no surface current but then we found that we would sometimes hit bugs on some phones so we decided to sync on pausing as we short on time last year. It might be possible to make this work but we should study the behavior across driver and Android versions.
Comment on attachment 697800 [details] [diff] [review]
patch

As much as I hate having this blocking I think we need to investigate the impact of this further before taking this patch.
Attachment #697800 - Flags: review?(bgirard) → review-
Try run for 442f6ecc0333 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=442f6ecc0333
Results (out of 73 total builds):
    success: 49
    warnings: 21
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-442f6ecc0333
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.