Skip to content
This repository has been archived by the owner on Mar 28, 2021. It is now read-only.

[real_hwc_wrapper] Do not throw on hwc->set() error #17

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

mariogrip
Copy link
Member

Based on what surfaceflinger does, we should simply ignore errors that set returns.

I see no reason why we should throw when set fails, as we already do hwc->perpare that will throw in case of a invalid state. In this case it seem to be a normal behavior to ignore as we can't check if the overlay is valid before committing. See: https://android.googlesource.com/platform/frameworks/native/+/android-7.1.1_r28/services/surfaceflinger/SurfaceFlinger_hwc1.cpp#1314

@peat-psuwit
Copy link
Contributor

peat-psuwit commented Nov 8, 2019

After I fixed the missing semicolon, now the test seems to fail at HwcWrapper.throws_on_set_failure, which is kind of expected.

However, by looking at the code, the exception removal might affect the (unexpected) external display disconnection. We might want to handle this in a different layer?

Based on what surfaceflinger does, we should simply ignore errors that
set returns.

I see no reason why we should throw when set fails, as we already do
hwc->perpare that will throw in case of a invalid state. In this case it
seem to be a normal behavior to ignore as we can't check if the overlay
is valid before committing. See:
https://android.googlesource.com/platform/frameworks/native/+/android-7.1.1_r28/services/surfaceflinger/SurfaceFlinger_hwc1.cpp#1314
@UniversalSuperBox
Copy link
Member

In my testing, behavior of external display connections or disconnections is not better or worse with this patch. Unity8 still must restart to recognize an external display and often crashes on losing it either way.

@mariogrip
Copy link
Member Author

In my testing, behavior of external display connections or disconnections is not better or worse with this patch. Unity8 still must restart to recognize an external display and often crashes on losing it either way.

This has no effect on external displays as they crash in a completely different place. im working on a patch for that, but this has no effect (as the throw would be ignored regardless)

Copy link
Member

@UniversalSuperBox UniversalSuperBox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for this seem fine, let's go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants