Skip to content
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

Viz Implementation for OSR #2575

Closed
magreenblatt opened this issue Jan 18, 2019 · 51 comments
Closed

Viz Implementation for OSR #2575

magreenblatt opened this issue Jan 18, 2019 · 51 comments
Labels
Framework Related to framework code or APIs osr Related to off-screen rendering task Task to be performed

Comments

@magreenblatt
Copy link
Collaborator

Original report by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


Chromium is currently replacing much of the old compositor code with a new system called Viz [1].
Right now we can still use the old path, but I expect them to remove that in a not too distant future (already disabled by default on desktop platforms [2]).

So we need a 'new' OSR implementation that is based on Viz.

[1] https://chromium.googlesource.com/chromium/src/+/master/services/viz/
[2] https://crrev.com/c/1374207

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


Was sick last week, but I started working on this and hope to have an initial PR ready at the end of this week or next week.

@magreenblatt
Copy link
Collaborator Author

Original changes by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


  • edited description

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


So a quick update on this, I have to say this turns out to be for me a lot more complicated than I initially expected.
It could be just I am just missing something but oh well.
The best explaination of the changes involved was this document [1] when using Viz Compositor, CompositorFrames are now longer sent from the render to the browser, but instead sent directly to the GPU process and then compositing happens directly onto the accelerated widget inside the render process.
This basically just completely prevents the current OSR approach, where we intercept the SubmitCompositorFrame calls. What I then tried was to add a new mojo IPC call that sents compositor frame metadata (also changed to get damage rect information) on every SubmitCompositorFrame call in the renderer, but right now I am facing issues of very high latency of frames being copied from the GPU to the browser process (talking sometimes multiple seconds here), I have yet to identify what is really going on there, but oh well.

This then kind of made me realize (or let's just say I am pretty sure) that we can't really do the current approach to shared texture support, but I will think about it, as all the output device stuff including the viz::Display no longer lives in the browser process but rather the render process.

If anyone has some input on this it would be greatly appreciated, but I will continue my endeavors regardless.
My current approach can be seen here [2]

[1] https://docs.google.com/document/d/1tFdX9StXn9do31hddfLuZd0KJ_dBFgtYmxgvGKxd0rY/edit
[2] https://bitbucket.org/xforce_dev/cef/commits/1e4bda98e58ca83c12176be33b8ee1a98d54fce9

@magreenblatt
Copy link
Collaborator Author

@xforce_dev Thanks for the update. I'll try to give feedback next week.

@magreenblatt
Copy link
Collaborator Author

I'm not sure what the best solution is here, especially with regards to shared textures. I've asked @wesselsga (the shared textures author) for feedback as well.

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


@magreenblatt thanks, I might just a bit in over my head with this here, but it's a great learning opportunity, digging through a lot of the chromium code and changes that are involved here.
I was however thinking about this a bit more and I think I have an idea on how to make it work, at least roughly, which I will try hopefully sometime this week.
Just want to finally finish my tests for the media access PR first (hopefully some time tonight), then I can devote more time for this.
At the end this is not immediately required, but I would personally prefer to have the Viz based version sooner rather than later, considering that the default for all desktop chrome platforms is now to use Viz.

@magreenblatt
Copy link
Collaborator Author

Original comment by Jakub Głuszkiewicz (Bitbucket: Jakub, GitHub: Jakub).


Guys do u think that software rendering path in osr and shared textures path will become obsolete soon in future versions?

@magreenblatt
Copy link
Collaborator Author

From kylechar@ on the Chromium/viz team:

If you want to do something similar to [having a custom SoftwareOutputDevice that owns an SkBitmap] then look at SoftwareOutputDeviceWinProxy and LayeredWindowUpdaterImpl. SoftwareOutputDeviceWinProxy draws into shared memory and then sends an IPC back to the browser process. LayeredWindowUpdaterImpl in the browser copies from shared memory to the HWND. We would definitely accept patches to make SoftwareOutputDeviceWinProxy and LayeredWindowUpdaterImpl more general base classes with windows specific implementation built on top. That way embedders could just extend the base class.

@magreenblatt
Copy link
Collaborator Author

FrameSinkVideoCapturer is also potentially an option when using hardware accelerated compositing. It does readback from GL and transports the pixels to the browser process. DevTools uses it to capture tab contents in a similar way.

Also, it's worth noting that Ozone does not support software compositing but instead has the GPU process rendering directly to the target surface. That's similar to the shared surface use case in CEF.

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


Thanks, that is actually quite helpful, I came across those before, but didn't quite understand how they specifically relate to some things, I am currently working on the Chromium 74 branch update, but will continue work on this again very soon (If all goes well sometime this weekend actually)

@magreenblatt
Copy link
Collaborator Author

@xforce_dev Thanks! I think it would be nice to trial the SoftwareOutputDeviceWinProxy and LayeredWindowUpdaterImpl generalization approach in CEF, and then submit those changes back to Chromium if it works out.

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


PR: https://bitbucket.org/chromiumembedded/cef/pull-requests/226
Although that is still WIP and just the finished stuff I had locally everything seems to be working fine,
except for shared texture, which I removed temporarily until my local new implementation is ready.

@magreenblatt
Copy link
Collaborator Author

Issue #2682 was marked as a duplicate of this issue.

@magreenblatt
Copy link
Collaborator Author

Issue #2722 was marked as a duplicate of this issue.

@magreenblatt
Copy link
Collaborator Author

Update to Chromium version 76.0.3809.0 (#665002)

OSR tests will be fixed by a follow-up merge of Viz support (see issue #2575).

→ <<cset cc0db5f166b6 (bb)>>

@magreenblatt
Copy link
Collaborator Author

Add initial Viz implementation for OSR (see issue #2575).

The old shared surface implementation has been removed and will need to be
re-implemented using the Viz code path.

→ <<cset ac2cc54e13ff (bb)>>

@magreenblatt
Copy link
Collaborator Author

Remove unused GetCompositor method and fix macOS compile error (see issue #2575)

→ <<cset cf87c88b0567 (bb)>>

@magreenblatt
Copy link
Collaborator Author

Original comment by Rob Sussman (Bitbucket: Rob Sussman).


I just tried building the 3809 branch but it failed with the error below (on mac). Should the above changeset cf87c88b0567 be applied to 3809 in addition to master?

../../cef/libcef/browser/osr/render_widget_host_view_osr.cc:1361:5: error: use of undeclared identifier 'browser_compositor_'
    browser_compositor_->UpdateVSyncParameters(
    ^

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


@{557058:24aafdc6-e7cd-4949-8383-58b12ab56885} yes

@magreenblatt
Copy link
Collaborator Author

Remove unused GetCompositor method and fix macOS compile error (see issue #2575)

→ <<cset d98abbb7927b (bb)>>

@magreenblatt
Copy link
Collaborator Author

Original comment by Juan Galeas (Bitbucket: jgaleas7, GitHub: jgaleas7).


is shared texture ready in 77 version?

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


@{5c51d64a5f99774462dc290f} not yet, no.

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


Hey, hopefully this is useful - I've basically just gotten it all working, but the attached is a re-implementation of the shared texture bits for the new viz osr approach. Compared to the earlier shared texture patch, this simplifies the chrome parts of the change quite a bit. Chrome now has enough shared texture code to handle a large portion of that earlier patch. Unfortunately, this means that cef is passing the textures back to the client slightly differently - chromium is internally using the dx11.1 nthandle/keyed mutex style textures, so the client needs to deal with the handles properly and lock/unlock the textures when used.

Patch should apply to the 3865 branch, and I should have it tested enough to submit a proper PR sometime next week.

@magreenblatt
Copy link
Collaborator Author

Original changes by Isaac Richards (Bitbucket: irichardsnv).


  • set attachment to "viz_osr_d3d.patch"

@magreenblatt
Copy link
Collaborator Author

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).


@{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3}
Thanks for the patch, I didn’t have the time yet to finish what I was working on which was trying to use the SharedImage work that has been going in chromium for quite a while [1] to maybe get a cross-platform implementation working but due to not having enough time available it’s not in a working state.

So thanks for doing this and hopefully we can get this merged so that we are back to the previous feature set of OSR.

[1] https://docs.google.com/document/d/12qYPeN819JkdNGbPcKBA0rfPXSOIE3aIaQVrAZ4I1lM/edit#heading=h.h8qur11p2eif

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


I actually tried for a quite a while to get the SharedImage stuff working first, but wasn’t able to. GLImageDXGI locks the texture when it’s bound, and I couldn’t figure out how to get the SharedImage interface to unbind the texture (and unlock it for the client to use) without destroying it.

The patch I posted is just using the GpuMemoryBuffer bits, though, and passing that to the OSR code in libcef, so there’s a good chance it’ll work with slight massaging on other platforms. The only windows specific code in there is to deal a bit with the handles in libcef and a small workaround for the brokenness of the dx11 keyed mutex design..

@magreenblatt
Copy link
Collaborator Author

Original comment by pkv (Bitbucket: pkvstream).


@{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} do you have a version of this patch for branch 3809 ? we’re quite interested in having shared textures reimplemented (we = obsproject) ; 3865 unfortunately is missing another feature which interests us (https://bitbucket.org/chromiumembedded/cef/commits/58e1149c7127314072903d3d45b9ba8b9fd2fc92)

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


Hey, bit of a delay on the accelerated texture stuff - I’ve got one last issue I’m trying to work out first with some rare flickering after a resize. In the meantime, though, I was able to get the accelerated path working on OSX as well, with very minor plumbing changes in CEF (basically getting the shared texture flag passed to RenderWidgetHostViewOSR). I’ll need to port the changes over to cefclient for that, though - need to change the GL code to use GL_TEXTURE_RECTANGLE instead of 2D, use the CGL/IOSurface APIs to get access to the shared texture, a couple other things...

I wasn’t planning on backporting this to 3809, but can take a look if I find the time.

@magreenblatt
Copy link
Collaborator Author

Original comment by pkv (Bitbucket: pkvstream).


@{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} great news for the support of macOS 👏 .

I tried to build your patch against 3865 but unfortunately the patch doesn’t apply against current branch. Do you have a more recent version of the patch ? Tried looking in your repo but it hasn’t been updated for a while.

edit: made a mistake in applying the patch. Discard

@magreenblatt
Copy link
Collaborator Author

Original comment by pkv (Bitbucket: pkvstream).


@{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} Tested your patch on cef 3904. It works very well. (Made some minor changes due to some chromium files being moved around from 3865 to 3904, the amended patch is there ).

edit: found some issues

@magreenblatt
Copy link
Collaborator Author

Original comment by Scott Maxwell (Bitbucket: scottmax, GitHub: scottmax).


Hi @{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} ,

Would it be possible for you to post another .patch file with your latest? I’d love to get the OSX support you mentioned. I’m working on building for Mac and Windows. I may have some bandwidth to move this forward if necessary but I’d like to start with the latest code you have.

Cheers,

Scott

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


Sure - here's my latest. Should be more stable than the last revision. I'm hoping to find the time to finish clean up and update to the latest branch and then submit a proper pull request in the next week or two.

It's still against the 3904 branch, and does not contain the cefclient changes required to use the hardware texture sharing on Mac.. To work on mac, the opengl code needs to change from GL_TEXTURE_2D to GL_TEXTURE_RECTANGLE, and once that's done, you can bind to the shared handles with something like:

#!c++

    mach_port_t port = (mach_port_t)(uintptr_t)(shared_handle);
    IOSurfaceRef m_cef_surface = IOSurfaceLookupFromMachPort(port);
    if (!m_cef_surface) {
        LOG(WARNING) << "failed to get io surface from: " << shared_handle;
        return;
    }

    CGLContextObj cgl_context = CGLGetCurrentContext();

    GLsizei surfw = IOSurfaceGetWidth(m_cef_surface);
    GLsizei surfh = IOSurfaceGetHeight(m_cef_surface);

    // bind to the texture we've created already.
    glBindTexture(GL_TEXTURE_RECTANGLE, m_texture_id);
    CGLError cgl_error = CGLTexImageIOSurface2D(cgl_context, GL_TEXTURE_RECTANGLE,
            GL_RGBA, surfw, surfh, GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, m_cef_surface, 0);
    if (cgl_error != kCGLNoError) {
        LOG(WARNING) << "CGLTexImageIOSurface2D: " << cgl_error;
    }
    glBindTexture(GL_TEXTURE_RECTANGLE, 0);

@magreenblatt
Copy link
Collaborator Author

Original changes by Isaac Richards (Bitbucket: irichardsnv).


  • set attachment to "viz_osr_d3d_3904_14Jan.patch"

@magreenblatt
Copy link
Collaborator Author

Original comment by Scott Maxwell (Bitbucket: scottmax, GitHub: scottmax).


Thanks! I’m porting these changes into Electron. I’ll see if I can figure out the Mac changes and if I am able to accomplish anything useful, I’ll figure out how to communicate the changes back.

@magreenblatt
Copy link
Collaborator Author

Original comment by Romain Caire (Bitbucket: Romain Caire).


Thanks a lot @issac Richards. Could this be (with minimal changes) be also used in an CEF OSR app running on Linux ?

@magreenblatt
Copy link
Collaborator Author

Original comment by pkv (Bitbucket: pkvstream).


the bug i Mentioned is fixed in the new patch @{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} ! Hope you can PR soon. Thanks for this.

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


@{557058:1abc75c9-ed35-467b-9d29-aa6fd2ae20fb} - Theoretically, yes, it should work with minimal modifications to the cef/chromium bits on Linux. You’d have to modify the libcef/browser/native/browser_platform_delegate files as I did for the Mac support to plumb through the use_shared_texture_flag on linux, and then in libcef/browser/osr/host_display_client_osr.cc’s OnAfterFlip() function, add stuff to unwrap the gfx::GpuMemoryBufferHandle to get the platform specific handle of the shared texture/data. Then on the client, your starting point for the changes would be somewhere around chromium/src/ui/gl/gl_image_native_pixmap.cc - would need to figure out how to go from the NativePixmapHandle you get in the GpuMemoryBufferHandle to something usable in opengl land..

@magreenblatt
Copy link
Collaborator Author

Original comment by Romain Caire (Bitbucket: Romain Caire).


Cool, thanks a lot @issac Richards . I’ll try that as soon as I have some spare time. Would be very interesting to see it working :slight_smile:

@magreenblatt
Copy link
Collaborator Author

Original comment by David Morasz (Bitbucket: microdee, GitHub: microdee).


I’ve tested out @{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} 's patch on windows on branch 3904 (with Release x64 config), unfortunately cefclient now only displaying white, instead of the proper content and has painting artifacts when resizing 😞 . Cefclient was run with --off-screen-rendering-enabled --shared-texture-enabled arguments, but even without shared textures it’s not displaying anything. Can I miss something?

As an extra I was generating projects with

set GN_DEFINES=use_jumbo_build=true is_component_build=true proprietary_codecs=true ffmpeg_branding=Chrome
set GN_ARGUMENTS=--ide=vs2017 --sln=cef --filters=//cef/*
set GYP_DEFINES=proprietary_codecs=1 ffmpeg_branding=Chrome
call cef_create_projects.bat

@magreenblatt
Copy link
Collaborator Author

Original comment by Riku Palomäki (Bitbucket: riku_palomaki).


I updated the patch by @{557058:a2ba0474-dc8c-446a-a7ba-63ea519588f3} on 3945 branch: https://bitbucket.org/riku_palomaki/cef/commits/137be9414bc47ba4351d927e20046335480e475a. I’m using this in our OpenGL based app with WGL_NV_DX_interop extension to use the shared D3D11 texture in an OpenGL context, which kind of works. However, I noticed couple of issues:

  • If our application can’t keep up with the browser engine, AcquireSync call in GLImageDXGI::BindTexImage will fail which seems to break everything on CEF side and eventually gets acquire/release calls out of sync and deadlocks the application. I fixed this by forcing the BindTexImagecall to wait infinitely: https://bitbucket.org/riku_palomaki/cef/commits/98880f6988aff76616e60dbfb498fdcf239c321a - this also makes sure the browser doesn’t run any faster than the actual client application.
  • Web pages that use post-processing effects like filter: blur(10px);don’t work at all and render as white windows, maybe this is what @{557058:839a108b-3556-4440-a35f-de23075d080f} saw as well. Easy way to reproduce this: cefclient.exe --off-screen-rendering-enabled --shared-texture-enabled --enable-gpu --url=https://www.autodraw.com/
  • For some reason wglDXRegisterObjectNV always fails without a call to wglDXSetResourceShareHandleNV which shouldn’t be needed at all. Also after ~10 mins nvidia drivers crash when calling wglDXSetResourceShareHandleNV. These GL DX interop issues are probably unrelated to this CEF patch.

@magreenblatt
Copy link
Collaborator Author

Original comment by Isaac Richards (Bitbucket: irichardsnv).


Thanks! - I forgot to comment here, but there’s an open pull request https://bitbucket.org/chromiumembedded/cef/pull-requests/285/reimplement-shared-texture-support-for-viz/diff with it updated to 3945 as well.

I wasn’t sure what to do about the acquiresync in gl_image_dxgi having an immediate timeout, like you’ve mentioned - since that code’s used for a number of other things inside of chromium, I couldn’t easily determine the side-effects of making it an infinite wait. One potential safe workaround would be to lock/unlock the handle inside of the new gl_output_surface_external with an infinite timeout after the texture comes back from the client, but before it’s handed back to the queue for chromium to render to..

I’ll take a look at the post-process thing.

@magreenblatt
Copy link
Collaborator Author

Original comment by Riku Palomäki (Bitbucket: riku_palomaki).


🤦‍♂️ Ah, I didn’t notice that merge request. It’s great that you are working on it!

It seems that GLImageDXGI uses triple buffering, I think in normal cases inside Chromium it’s not possible to get into a situation where AcquireSync wouldn’t just immediately succeed, so I would like to believe that the change to infinite timeout is safe.

@magreenblatt
Copy link
Collaborator Author

Original comment by David Morasz (Bitbucket: microdee, GitHub: microdee).


I can confirm Isaac Richards’ pull request works fine except that autodraw website was glitching as mentioned before, however it might not be caused solely by the CSS filters as Riku Palomäki said, because they worked perfectly well for me in the w3school live examples (blur and dropshadow or any combination of multiple ones as well).

@magreenblatt
Copy link
Collaborator Author

Original comment by Jon S (Bitbucket: sirbrialliance, GitHub: sirbrialliance).


Issue #2960 was marked as a duplicate of this issue.

@magreenblatt
Copy link
Collaborator Author

Original comment by Felix Faust (Bitbucket: Felix Faust).


any news on this?

@magreenblatt
Copy link
Collaborator Author

Original comment by Qiang Li (Bitbucket: Qiang Li).


Now I am using the latest version 86.0.241(Cefsharp.Wpf), facing the same problem, expected this version can resolve but i am wrong.
And I am search a lot of same issue but the result was end up with nothing conclusive.
Finally I step by step debuged and found that when closing the window which browser is in, sometime the page stacked( process is killed), so I thinked if window's close method caused the irregular killing? And I override the closing method as follows:
First, My page's image is below:
1: Window1View -> Window1Model -> browserModel
2: Window2View -> Window2Model -> browserModel
the browserModel is moving from one to another bettwen window1 and 2

Use this way I haven't see the above process killed problem (cef's log was no error even if page stacked)

        private void Window1_OnClosing(object sender, CancelEventArgs e)
        {
            // when click close button not to close, just hide the window1, and browserModel is reused in window2
            if (sender.As<Window1View>().Visibility == Visibility.Visible)
            {
                sender.As<Window1View>().Visibility = Visibility.Hidden;
                e.Cancel = true;
                Window2Model.browserModel = Window1Model.browserModel;
                Window2View.Show();
            }
        }

But I don't know if it is the right way to resolve the problem, anyone can give same advice ?

@magreenblatt magreenblatt added the osr Related to off-screen rendering label Mar 12, 2023
S1artie pushed a commit to GEBIT/cef that referenced this issue Aug 10, 2023
OSR tests will be fixed by a follow-up merge of Viz support (see issue chromiumembedded#2575).
S1artie pushed a commit to GEBIT/cef that referenced this issue Aug 10, 2023
…).

The old shared surface implementation has been removed and will need to be
re-implemented using the Viz code path.
@magreenblatt
Copy link
Collaborator Author

Related PR for OnAcceleratedPaint support: https://bitbucket.org/chromiumembedded/cef/pull-requests/734

@reitowo
Copy link
Contributor

reitowo commented Apr 24, 2024

Thanks! Marshall, could you backport the commit to M124 branch when possible? It was confirmed to work on that.

magreenblatt pushed a commit that referenced this issue Apr 28, 2024
Adds support for the OnAcceleratedPaint callback. Verified to work
on macOS and Windows. Linux support is present but not implemented
for cefclient, so it is not verified to work.

To test:
Run `cefclient --off-screen-rendering-enabled --shared-texture-enabled`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Related to framework code or APIs osr Related to off-screen rendering task Task to be performed
Projects
None yet
Development

No branches or pull requests

2 participants