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

Add screencopy dmabuf backend #122

Conversation

CheerfulPianissimo
Copy link
Contributor

This PR will contain the changes required for my Google Summer of Code 2024 project with waycrate as described here: https://cheerfulpianissimo.github.io/notes/GSOC/GSOC.html

@CheerfulPianissimo
Copy link
Contributor Author

It isn't clear to me why that build is failing. Seems to be some issue with the wayland deps?
A more detailed description of the changes so far is available at https://cheerfulpianissimo.github.io/notes/GSOC/GSOC%20Devlog%20(June).html#!week-4 :

Reading the code, I discovered that there are three levels of abstraction in libwayshot:

  • The screenshot_* series of functions: These are the public API and mostly wrap around one of the capture_* series of functions.
  • The capture_frame_copy* and capture_output_frame_shm_fd series of functions: capture_output_frame_shm_fd is a part of the public libwayshot API that allows for low-level access to the screencapture data in the form of file descriptors and a FrameGuard struct that contains a WlBuffer. The capture_frame_copy* functions handles the creation of a FrameCopy struct which is a sort of intermediate data structure between raw shared memory and a DynamicImage, these functions are used by the screenshot* function layer above and are not public.
  • The capture_output_frame_get_state and capture_output_frame_inner functions: These two functions deal with the nitty-gritty of interacting with the wlr-screencopy protocol. The primary data structure here is CaptureFrameState.
    • capture_output_frame_get_state sets up a zwlr_screencopy_manager_v1 with the correct sub-region and the output to be captured to get a zwlr_screencopy_frame_v1
    • capture_output_frame_inner manages the copy of frame data from the created zwlr_screencopy_frame_v1 into the shared memory buffers.
    • capture_output_frame_shm_fd and it's sister function capture_output_frame_shm_from_file call the above two functions one after the other to handle everything to do with wlr-screencopy.

First Draft API Implementation

For the first phase I decided to create dmabuf version of the lower two levels of abstraction. My goal was to create a dmabuf equivalent for capture_output_frame_shm_fd which would provide low level access to the dma buffer objects and a wl-buffer.

To this goal, I created the new function variants capture_output_frame_get_state_dmabuf and capture_output_frame_inner_dmabuf to replicate the lowest layer and capture_output_frame_dmabuf to replicate the public API on the level above. capture_output_frame_dmabuf would return a GBM BufferObject, a FrameGuard containing a WlBuffer and the frame format descriptor.

Two new structs DMAFrameFormat and DMAFrameGuard were created as counterparts to FrameFormat and FrameGuard.

For the most part adapting the code from the MVP into these functions was straightforward with the shm variants as a reference.

One part I have some trouble with is error handling. Using unwrap() in library code is generally frowned upon but I haven't figured out a good way to pass up errors to the caller without running into compiler errors like "? couldn't convert the error to error::Error
the question mark operation (?) implicitly performs a conversion on the error value using the From trait". This requires some research.

The next phase will be testing. I will need some way to test the new API, ideally without having any VRAM->CPU copying. The simplest plan is to start with a basic wayland window demo and attach the WlBuffer obtained from the API to a WlSurface and see what the result looks like.

@CheerfulPianissimo CheerfulPianissimo marked this pull request as ready for review August 25, 2024 05:47
@Shinyzenith
Copy link
Member

Apart from these minor changes, everything else looks good!

@Shinyzenith
Copy link
Member

Ci is failing @CheerfulPianissimo

@CheerfulPianissimo
Copy link
Contributor Author

The addition of wayland-sys as a dependency is causing it to fail I think. That library requires a preinstalled wayland system library.

@CheerfulPianissimo
Copy link
Contributor Author

fixed that but it seems we need something for egl too.

@CheerfulPianissimo
Copy link
Contributor Author

alright, the build is passing now but I think it may be wise to document the new deps somewhere and/or feature-gate it.

@Shinyzenith
Copy link
Member

We can have it as part of changelog or release notes. Sounds good?

@CheerfulPianissimo
Copy link
Contributor Author

Yeah that's fine by me. I already have a draft changelog for this at https://cheerfulpianissimo.github.io/notes/GSOC/GSoC%20Changelog.html

@Shinyzenith Shinyzenith merged commit 5d69000 into waycrate:freeze-feat-andreas Aug 28, 2024
3 checks passed
@Shinyzenith
Copy link
Member

Congratulations and thank you for your service to the open source community @CheerfulPianissimo

Shinyzenith added a commit that referenced this pull request Feb 12, 2025
* Refactor: introduce FrameGuard, ScreenCapturer, and Logical/EmbeddedRegion

* Add freeze functionality

* Move to clap_derive so CLI arguments are typed



* Renames `clap.rs` to `cli.rs` because otherwise there's confusion between the `clap` crate and local module.
* `Cli` struct added that almost identically represents the current state of the CLI with no logical changes.

---

## `--help` Comparison

Before: https://gist.github.com/AndreasBackx/5945b366e989159f4669e7ba30c13239

After:  https://gist.github.com/AndreasBackx/8929c8bde080eac0cafd33128210b0cc

Diff (ignoring whitespace changes due to table alignment):
```diff
1c1
< Screenshot tool for compositors implementing zwlr_screencopy_v1.
---
> Screenshot tool for wlroots based compositors implementing the zwlr_screencopy_v1 protocol.
9d8
<   -c, --cursor                      Enable cursor in screenshots
10a10
>   -c, --cursor                      Enable cursor in screenshots
12c12
<   -l, --listoutputs                 List all valid outputs
---
>   -l, --list-outputs                List all valid outputs
14c14
<       --chooseoutput                Present a fuzzy selector for outputs
---
>       --choose-output               Present a fuzzy selector for outputs
16a17
>
```

You can see that the only changes are:
- About is longer, this is now using the value from Cargo.toml instead of a duplicate text that was shorter.
- Some have a dash where the English words would have a space, e.g: "list-outputs" instead of "listoutputs". I've also made the old still work via an alias: https://gist.github.com/AndreasBackx/6025e91844e3d766d4264a01ae4d1a71

This seems like a tiny improvement? I plan to make further changes later, but I want to keep PRs separate.

* Improve CLI design


This "improves" (and that is subjective) the design of the CLI. I am aiming to get some feedback on what people think of the new design:

```
Screenshot tool for wlroots based compositors implementing the zwlr_screencopy_v1 protocol.

Usage: wayshot [OPTIONS] [OUTPUT]

Arguments:
  [OUTPUT]
          Where to save the screenshot, "-" for stdout. Defaults to "$UNIX_TIMESTAMP-wayshot.$EXTENSION"

Options:
      --log-level <LOG_LEVEL>
          Log level to be used for printing to stderr

          [default: info]
          [possible values: trace, debug, info, warn, error]

  -s, --slurp <SLURP_ARGS>
          Arguments to call slurp with for selecting a region

  -c, --cursor
          Enable cursor in screenshots

      --encoding <FILE_EXTENSION>
          Set image encoder, if output file contains an extension, that will be used instead

          [default: png]
          [aliases: extension, format, output-format]

          Possible values:
          - jpg: JPG/JPEG encoder
          - png: PNG encoder
          - ppm: PPM encoder
          - qoi: Qut encoder

  -l, --list-outputs
          List all valid outputs

  -o, --output <OUTPUT>
          Choose a particular output/display to screenshot

      --choose-output
          Present a fuzzy selector for output/display selection

  -h, --help
          Print help (see a summary with '-h')

  -V, --version
          Print version
```

The main changes are:
1. `--debug` is now `--log-level` because this makes it easy to select more specifically what log level to use. I considered using `-v`, `-vv`... to increase verbosity but the `clap-verbosity-crate` uses `log` and not `tracing`. We could use it somewhat, but we'd pull in `log` (which seems fine) and we'd need to map from `log`'s Level to `tracing`'s Level enums (they use inverse ordering).
2.  `--stdout` and `--file` has been made an optional positional argument. This because it's what other CLIs often do and I wasn't sure what to call the option otherwise because `--output` and `-O`/`-o` is often what others use but we use it here to refer to displays/monitors/Wayland outputs. This avoids that confusion hopefully and I've also clarified this in the documentation.
    * Additionally if a path is given, its extension will always be used. So you cannot save `jpg` to `foo.png`. Perhaps this behaviour can be changed, though I don't see a reason to support this weird edge case? When is someone saving `png` to `jpg`?
3. `--extension` is `--encoding` with aliases like `extension`.

Again, let me know what you think.

* Remove explicit panics and exits

Let's bubble up errors instead of panicking and also not use `exit(...)` because it does not call destructors which might give issues.

* Make region, sizing, and positioning data structs reusable

We use different structs/fields to store the same three types of data everywhere:
- Position (x,y)
- Size (width,height)
- Region(position, size)

This makes it so they all reuse the same information.

* First version of scaling fix. (#85)

* chore: remove useless roundtrip (#105)

* feat(clipboard): implement clipboard integration (#91)

* feat(clipboard): implement clipboard integration

Add the --clipboard flag and implement functionality to make image available on the clipboard using wl-clipboard-rs.

* style(format): apply code formatting to cli.rs

* feat(clipboard): implement fork wait for clipboard

Add functionality offering image on clipboard persistently in the background

* feat(clipboard): inform user about wayshot persisting in background with --clipboard

* feat(clipboard): use tracing::warn instead of print if fork fails

* feat(clipboard): Switch from the fork crate to the nix crate for daemonization

* style(format): code formatting to wayshot.rs

* style(typo): corrected a typo in the comments

* docs(ManPage): Fix capitalization issue

Signed-off-by: Shinyzenith <[email protected]>

* docs: Document clipboard interaction

Signed-off-by: Shinyzenith <[email protected]>

* chore: Update Cargo.lock

Signed-off-by: Shinyzenith <[email protected]>

* feat: added time_stamp flag (#93)

* feat: added time_stamp flag

* Syntax Refactor and filename format fixed

* file name format fixes

* wayshot-{timestamp}.{extension} as default filename

* chore: Update Cargo.lock

Signed-off-by: Shinyzenith <[email protected]>

---------

Signed-off-by: Shinyzenith <[email protected]>
Authored-by: rachancheet <[email protected]>
Co-authored-by: Shinyzenith <[email protected]>

* chore: Update Cargo.lock

Signed-off-by: Shinyzenith <[email protected]>

* feat: Add support for webp (#98)


Signed-off-by: Shinyzenith <[email protected]>

---------

Signed-off-by: Shinyzenith <[email protected]>
Co-authored-by: Shinyzenith <[email protected]>

* refactor(libwayshot): Reduce allocations (#99)

* refactor: remove unnecessary to_string()

* refactor: fix clippy warnings

* refactor: remove 1 level of indirection via &Vec<T> -> &[T]

Typically `&Vec<T>` isn't something you want when you need a read-only view
over a Vec, because it adds another level of indirection: Vec already stores
a pointer to heap-allocated buffer internally.

Using slices `&[T]` removes such unnecessary level of indirection and
is considered a cleaner design. It is cache friendlier and can be better
optimized by the compiler (not that it should matter in this case).

* feat: account for directories in file path (#96)

Signed-off-by: Shinyzenith <[email protected]>

---------

Signed-off-by: Shinyzenith <[email protected]>
Authored-by: rachancheet <[email protected]>
Co-authored-by: Shinyzenith <[email protected]>

* fix: Clipboard flag ignored if path is qualified to file (#108)

* feat(clipboard): fix issue #106 clipboard flag ignored if path is qualified to file

* feat(clipboard): clarify flag description of --clipboard

* feat(clipboard): enable multiline comment description for --clipboard feature flag

Signed-off-by: Shinyzenith <[email protected]>

* feat(clipboard): reduce buffer allocations for encoding image

* feat(clipboard): improve code quality

* feat(clipboard): code style change: perform match inside function call

---------

Signed-off-by: Shinyzenith <[email protected]>
Co-authored-by: Shinyzenith <[email protected]>

* chore: make clippy happy (#111)

* feat(clipboard): fix interaction with freeze and select feature: Issue #109 (#110)

* feat(clipboard): fix interaction with freeze and select feature:  Issue #109

* feat(clipboard): handle callback fails

* Destroy layer shell surfaces

* [fix] unmap layer shell surfaces before destroying them

* Implement missing conversion from string to webp enum (#121)

* Replace nix with rustix (#120)

* Update documentation with changes in freeze-feat (#116)

* [docs] update manpage (1) with changes in freeze-feat

* [fix] make arguments to slurp optional

* [docs] update manpage(7) with cli changes

* [docs] update README with new cli

* Add screencopy dmabuf backend (#122)

* [feat] rough MVP for screencpy+dmabuf

* [feat] refactor MVP into libwayshot - create new constructor

* [refactor] first draft of dmabuf API

* [refactor]
- Add error handling
- Add example/demo for wayshot dmabuf API

* [feat] import wayland-egl-ctx for use as MVP for dmabuf import functionality

* [fix] correct modifier_lo parameter in waymirror-egl MVP

* [feat] get waymirror-egl dmabuf MVP demo working

* [feat] refactored dmabuf->eglImage API into libwayshot

* [feat] - Implemented clean dropping of EGLImage
-  Improved API docs for dmabuf functions

* [fix]  libwayshot build error fixed

* [fix] remove hardcoded GPU path from libwayshot constructor

* [fix]] remove reduntant dmabuf_to_texture call from waymirror-egl

* [docs] document WayshotConnection dmabuf constructor

* [feat] Added helper/wrapper function to convert screencapture EGLImages into GL textures

* [fix] improved logging in dmabuf API code

* [doc] update waymirror-egl Readme

* [fix] change logging level in waymirror-egl to Debug

* [fix] remove unnecessary .gitignore in waymirror-egl

* [ci/cd] attempting to fix the build

* [ci\cd] add libegl system deps to fix github CI

* [fix] remove unused egl_image struct field in Waymirror demo

* fix: I find out a smart way to fix scale problem (#128)

* fix: I find out a smart way to fix scale problem

* Update libwayshot/src/lib.rs

Co-authored-by: Aakash Sen Sharma <[email protected]>

---------

Co-authored-by: Aakash Sen Sharma <[email protected]>

* Fix for 'cargo test' (#114)

* fixes for 'cargo test' & bump flake.lock due to outdated cargo

* commit to pass `cargo fmt -- --check` test

* add `cargo test` to Makefile

---------

Co-authored-by: id3v1669 <[email protected]>

---------

Signed-off-by: Shinyzenith <[email protected]>
Co-authored-by: Access <[email protected]>
Co-authored-by: Andreas Backx <[email protected]>
Co-authored-by: Andreas Backx <[email protected]>
Co-authored-by: Shivang K Raghuvanshi <[email protected]>
Co-authored-by: Sooraj S <[email protected]>
Co-authored-by: rachancheet <[email protected]>
Co-authored-by: Gigas002 <[email protected]>
Co-authored-by: Sergey A <[email protected]>
Co-authored-by: id3v1669 <[email protected]>
Co-authored-by: id3v1669 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants