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

feat: encoder pro max #37

Merged
merged 23 commits into from
Apr 23, 2024
Merged

feat: encoder pro max #37

merged 23 commits into from
Apr 23, 2024

Conversation

clearlysid
Copy link
Collaborator

This PR is a big one. It gets rid of FFmpeg in favour of OS-based encoders for the preview generation. This makes our binary even smaller, and makes the distribution and bundling proceses easier. Under the hood, It uses a custom Swift package for macOS and windows-capture encoder for Windows

TODOs:

  1. clean up swift code
  2. create BGRA encoding for mac encoder (currently it only works on YUV)
  3. improve quality, colour config and codec of mac encoder
  4. try building final binary

@clearlysid
Copy link
Collaborator Author

For the macOS encoder, we can use my https://github.com/clearlysid/bh-encode/tree/main as a reference point for testing. Currently the problem is that I have only created the encoder for YUVFrames captured in Mac. However, for Micro's usecase Gifski will prefer to have BGRA (i think, need to confirm).

The encoder will not work until we support that format also. This PR cannot be merged to main until that feature is complete.

@clearlysid
Copy link
Collaborator Author

oops

Screenshot 2024-04-23 at 12 17 03 AM

@clearlysid
Copy link
Collaborator Author

atleast it is fast 😂

@anubhavitis
Copy link
Contributor

Not blurry at my end 🤔
Screenshot 2024-04-23 at 12 33 44 AM

@anubhavitis
Copy link
Contributor

Gif encoding is working as well :)
HM-240423-1234AM

@clearlysid clearlysid requested a review from anubhavitis April 22, 2024 19:08
@clearlysid clearlysid marked this pull request as ready for review April 22, 2024 19:08
@Pranav2612000
Copy link
Collaborator

oops

Screenshot 2024-04-23 at 12 17 03 AM

Was this resolved? If not, I can help.

@clearlysid
Copy link
Collaborator Author

It is resolved on my machine, yup. Changed byteStride to width * 4 like we'd done somewhere in scap. The Stride somehow seems to be platform dependent though, I never figured out how it works.

For eg: in @anubhavitis's machine it was working properly even before I made the stride fix. Does that mean his version would be broken now? 🤔

@clearlysid clearlysid merged commit f30bb99 into main Apr 23, 2024
@Pranav2612000
Copy link
Collaborator

It is resolved on my machine, yup. Changed byteStride to width * 4 like we'd done somewhere in scap. The Stride somehow seems to be platform dependent though, I never figured out how it works.

For eg: in @anubhavitis's machine it was working properly even before I made the stride fix. Does that mean his version would be broken now? 🤔

We can get on a call to debug this if this doesn't work.
I think the * 4 was for each of RGBA.
If I'm not wrong the problem arises because the capturer adds extra black pixels if the width is not a multiple of 4. Trying to move the original width in this scenario would mess everything up.

Again, if it's working no need to touch it. But if we encounter errors, I'll be happy to help. I'll also test it out with different values.

@clearlysid clearlysid deleted the feat/encoder-pro-max branch April 23, 2024 13:50
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