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

Fix OnAcceleratedPaint2() not being called when there's no new texture #1

Open
wants to merge 1 commit into
base: 5060-shared-textures
Choose a base branch
from

Conversation

elvissteinjr
Copy link

Description

This pull request fixes libcef_dll wrapped calls of OnAcceleratedPaint2() failing parameter checks if shared_handle is null even when new_texture is false (where this is expected). It's pretty much just adjusting the check to how it was in the initial on-accelerated-paint2 branch.

Motivation and Context

Without this change, OnAcceleratedPaint2() is only called when the texture changed. This currently does not appear to be a problem for obs-browser, admittedly, but since it silently fails in release builds and for the sake of correctness, I believe it's worth fixing.
It might also help in the future when there's going to be some kind of compositing going on (like overlaying PET_POPUP elements as there's a todo-comment for).

And while I don't expect this to be intended or supported use in any way, I've been making use of this CEF fork in my own project. If possible I'd like to avoid maintaining a fork on top of a fork for a couple of changed lines, but I realize I'm on my own making use of your work here.

How Has This Been Tested?

I've tested the change on Windows 10 with obs-browser, as well as in my own project (Desktop+ Browser), by checking if the browser output still works while making sure hardware acceleration and OnAcceleratedPaint2() is being used.

As there are no functional changes for obs-browser, there's not much of an effect there right now, except that OnAcceleratedPaint2() gets called for every frame update again (but always just returns when new_texture is false right now).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format. (I assume that wouldn't apply to CEF's code?)
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation. (not that there is any)

@WizardCM
Copy link
Member

I'm personally not opposed to merging this if this'd definitely make the PET_POPUP composition easier to do, however it's worth noting that 5060 is likely the last build that'll support using this renderer, so this code will likely be short-lived.

@jp9000
Copy link
Member

jp9000 commented Aug 13, 2022

If you don't mind me asking, what makes it a problem in your case?

@elvissteinjr
Copy link
Author

If you don't mind me asking, what makes it a problem in your case?

Performance and frame pacing. In my case the application is a SteamVR overlay, typically running alongside a VR game. In this scenario I don't control the final output, but instead just submit a texture to the SteamVR compositor whenever I feel like it. Ideally that is only when there's a change in the browser output as there's performance overhead with each submission (it doesn't use the shared texture directly but copies it).

Then there's the frame pacing part. Chromium does a good job at not rendering when not necessary, so an idle page causes zero updates and playing a 24 fps video for example results in 24 updates per second.
The VR headsets my applications runs with can have a variety of refresh rates too (60, 72, 80, 90, 120, 144 Hz are typical). Submitting the shared texture at fixed intervals works, but is wasteful and can miss its mark. To make matters worse, on the SteamVR-end there's no option to 100% sync frame timing with the compositor as an overlay app (the VR foreground apps/games have that of course).
Using OnAcceleratedPaint2() as a signal for a new frame to process is ideal and seems like it'd also be intended. I'm fairly new to CEF dev but I don't think there's even another way to get that info either?

I'm not too familiar with OBS' internals, but from what I gathered glancing around it's fine taking the shared texture when it needs it when compositing the scene, so it doesn't need to care when updates to the texture happen compared to my app.

To clarify again, this worked fine in the initial OnAcceleratedPaint2() patch for CEF 4638, so this pull request merely restores the check to be the same as on that branch (I assume the adjustment of the generated library wrapper was overlooked this time). I just ran into this when trying to upgrade the CEF version on my end when 5060 was patched for OBS.

Julusian added a commit to CasparCG/server that referenced this pull request Nov 14, 2023
Uses Julusian/cef@f60566c which is obsproject/cef#1 rebased on top of some more recent changes to the 103 branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants