826607 - don't block while pausing and resuming the compositor

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

It's not looking so green any more.

Comment 5

[Mozilla RelEng Bot]

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/[e-mail]-96d952bef2c9

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-e33bbeee63e4

Comment 7

[Mozilla RelEng Bot]

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/[e-mail]-9d549ad7d400

Comment 8

[Mozilla RelEng Bot]

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/[e-mail]-e33bbeee63e4

Comment 10

[Benoit Girard (:BenWa)]

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.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(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?

Comment 12

[Benoit Girard (:BenWa)]

(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 13

[Benoit Girard (:BenWa)]

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.

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-442f6ecc0333