Refactor: introduce FrameGuard, ScreenCapturer, and Logical/EmbeddedRegion #71
Conversation
AndreasBackx
changed the title
AndreasBackx
force-pushed
the
pr71
branch
2 times, most recently
from
3368d06 to
ac951cb
Compare
AndreasBackx
force-pushed
the
pr71
branch
2 times, most recently
from
4077a85 to
becea47
Compare
|
Another question that I had, can we not screencopy all outputs when freeze is not asked for? Freeze can be an opt-in feature via a flag. This is for the people who'd rather buy performance over freeze. Also do we still scrcpy all outupts even when only one output is specifically asked for? |
|
This would also be a good opportunity to introduce a Changelog for libwayshot for downstream clients as we are making breaking changes to the API very often. |
| frame.buffer_done.store(true, Ordering::SeqCst); | ||
| } | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
Once the flag event removal is reverted, this needs to be changed back to unreachable.
There was a problem hiding this comment.
What I'd want to avoid however, is when there is a newer version of screencopy with a new event, that we'd reach this leading to an error. If the event enum is exhaustive, we can remove the _ => {} and be sure this won't happen anyhow. If it's not, I'd want to keep _ => {} and not _ => unreachable!() as it might lead to errors in the future, no?
There was a problem hiding this comment.
I understand where you're coming from. I believe we request the exact version of screencopy we want from the queuehandle anyways, when we update the version of scrcpy, the compiler should warn us of an unhandled cases right as it's an exhaustive enum? So maybe we can just expand the handler with some polyfill (eg: //TODO: handle this later/ignored event) for the cases we don't currently handle?
There was a problem hiding this comment.
I believe we request the exact version of screencopy we want from the queuehandle anyways
Now that you mention this, we need to update the code afaik to say version X or higher as I think right now if a new version is introduced, we won't support a compositor that reports a higher version even though it should also support the older version. 🤔 Though unrelated to this PR.
when we update the version of scrcpy, the compiler should warn us of an unhandled cases right as it's an exhaustive enum?
It might warn us (though don't think so because we have _ => {}), a newer version will still hit _ => unreachable!(). We simply cannot guarantee this is unreachable with a non exhaustive enum.
| frame.buffer_done.store(true, Ordering::SeqCst); | ||
| } | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
What I'd want to avoid however, is when there is a newer version of screencopy with a new event, that we'd reach this leading to an error. If the event enum is exhaustive, we can remove the _ => {} and be sure this won't happen anyhow. If it's not, I'd want to keep _ => {} and not _ => unreachable!() as it might lead to errors in the future, no?
Take your time! thanks for your contributions, there's no rush. My apologies if I reviewed something incorrectly without proper understanding, I'll give it all another go after the info is up so I can properly guage the change surface. |
AndreasBackx
changed the title
AndreasBackx
force-pushed
the
pr71
branch
5 times, most recently
from
1a3e051 to
b5a797e
Compare
AndreasBackx
force-pushed
the
pr71
branch
2 times, most recently
from
790bf8b to
2649b28
Compare
|
I think this PR is ready to be merged. Any comments @Decodetalkers? I'll start landing this stack entirely. |
| } | ||
|
|
||
| fn create_frame_copy( | ||
| pub fn capture_frame_copies( |
There was a problem hiding this comment.
since this function is public ,so I think add docs is needed
There was a problem hiding this comment.
We might be able to keep this private. I forgot if it was used, I might've forgot to undo this.
I also think this pr is ok, maybe document can added later |
|
I will be closing down the rest of the stack since the top of the stack has been merged with all the changes included 😄 |
Refactor: introduce FrameGuard, ScreenCapturer, and Logical/EmbeddedRegion
Stack created with Sapling. Best reviewed with ReviewStack.