New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve OSR API (master) #11728

Merged
merged 7 commits into from Feb 12, 2018

Conversation

Projects
None yet
5 participants
@brenca
Copy link
Member

brenca commented Jan 25, 2018

This PR addresses bugs that came up in #11715 and contains some other small improvements

  • Fixes StopPainting not working when using GPU OSR codepath (#11715)
  • Fixes a bug where calling StartPainting inside the OnPaint callback in GPU OSR mode would try to generate another frame, essentially calling the callback recursively (#11715)
  • Switches to the nullAcceleratedWidget to avoid showing the GPU rendered OSR surface on the dummy window
  • Raises the maximum frame rate to 240 to prepare for modern high refresh rate displays
  • Switches to calling SetAuthoritativeVsyncInterval on the Compositor instead of it's vsync_manager
  • Moves the OSR related API calls to the OffscreenWebContentsView, because that gets created with the WebContents, while the OffscreenRenderWidgetHostView is created a bit later. This allows calls to take effect even if they are made before the first paint event arrives.

@brenca brenca requested review from ckerr and alexeykuzmin Jan 25, 2018

@brenca brenca requested a review from electron/reviewers as a code owner Jan 25, 2018

web_contents()->GetRenderWidgetHostView());
if (osr_rwhv)
osr_rwhv->SetPainting(true);
const auto* wc_impl = web_contents_impl();

This comment has been minimized.

@deepak1556

deepak1556 Jan 25, 2018

Member

This should be either const content::WebContentsImpl* wc_impl = or const auto* wc_impl = static_cast<content::WebContentsImpl*>() , we have a InspectableWebContentsImpl class which can cause confusion here. Will defer to others for the right choice, personally I prefer the second version. Any reason for the guard if (!wc_impl), is it possible for the webContents to be destroyed when this method is called ?

This comment has been minimized.

@brenca

brenca Jan 25, 2018

Author Member

Yeah, I suppose writing out the static_cast can make this a bit clearer. As for the guard, I don't actually see any way it could currently fail, I'm okay with removing it, it was just a precaution to avoid potential crashes later down the line.

@brenca

This comment has been minimized.

Copy link
Member Author

brenca commented Jan 29, 2018

The CI crash seems to be an instance of #9487, not related to the changes I made.

@ckerr
Copy link
Member

ckerr left a comment

I'll give a real review in the next few days when I have time to give this proper attention, but the first thing that sticks out to me is there aren't any tests to confirm that these changes work.

Could you please add some tests, e.g. to confirm the #11715 stop/start fixes?

@brenca

This comment has been minimized.

Copy link
Member Author

brenca commented Jan 30, 2018

@ckerr Yeah, sure, will do

@ckerr

This comment has been minimized.

Copy link
Member

ckerr commented Feb 3, 2018

@brenca 👍

@jkleinsc jkleinsc merged commit e6ac263 into electron:master Feb 12, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment