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-raw-pixel-buffer-to-image: skia impl #2

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

bnert
Copy link
Contributor

@bnert bnert commented Sep 7, 2020

Pixel buffer support has been added for the Skia backend.

By default, the memory for the image is copied over to be managed
by the SkBitmap instance within the image. This is to give the caller
safety when using a raw pointer after creating an image.

Pixel buffer support has been added for the Skia backend.

By default, the memory for the image is copied over to be managed
by the SkBitmap instance within the image. This is to give the caller
safety when using a raw pointer after creating a paintable pixmap.
@bnert bnert marked this pull request as draft September 7, 2020 21:52
@bnert
Copy link
Contributor Author

bnert commented Sep 7, 2020

@djowel here is the Skia implementation, following up on our conversation on discord/cycfi/elements#227

I do not have access to a Mac/macOS at the moment. Would you, or someone else be able to handle that chunk of work?

@djowel
Copy link
Member

djowel commented Sep 7, 2020

Wow, you are fast! :-)

Any chance you can do something with the Quartz-2D backend? :-) I probably shouldn't be pushing my luck. Haha :-)

I'll review ASAP.

@djowel
Copy link
Member

djowel commented Sep 7, 2020

I do not have access to a Mac/macOS at the moment. Would you, or someone else be able to handle that chunk of work?

Ah there. Of course, I can do that.

@djowel djowel self-assigned this Sep 7, 2020
lib/impl/skia/image.cpp Outdated Show resolved Hide resolved
@djowel
Copy link
Member

djowel commented Sep 7, 2020

Added some comments above ^. I'll look closer again later... Many thanks for your contribution!

@bnert
Copy link
Contributor Author

bnert commented Sep 7, 2020

For sure! Thanks for the code review!


uint32_t constexpr white = 0xffffffff;
// on little endian systems RGBA is formatted as ABGR for values
std::function<uint32_t()> black = []() { return is_little_endian() ? 0xff000000 : 0x000000ff; };
Copy link
Member

Choose a reason for hiding this comment

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

Come to think of it, infra/support.hpp should probably have a to_little_endian(x) and to_big_endian(x) set of overloads for different scalar types.

Copy link
Contributor Author

@bnert bnert Sep 7, 2020

Choose a reason for hiding this comment

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

Added an issue in cycfi/infra: cycfi/infra#12

- Moved to use function to determine api specific alpha/byte format
  type.
- Updated infra submodule to latest master, in order to have
  `is_little_endian` function for example.
- Updated copyright
@bnert bnert marked this pull request as ready for review September 7, 2020 22:32
@djowel
Copy link
Member

djowel commented Sep 8, 2020

Some things I have in mind, but I can add/implement later:

  1. The concept of the raw pixel buffer, instead of uint8_t* data cries out for an abstraction, so you do not have to do the array management every time as it presents itself as an abstracted 2D array of pixels. It should be similar to the buffer in infra (which should really be renamed as audio_buffer or generalized to handle pixel buffers as well). I'll deal with that later.
  2. It should be unified with the uint32_t const* image::pixels() member function. I'll also deal with this later, but do take note that the API as presented in this PR is subject to change.

For now, I'm good with merging this PR. Thanks for working on this!

@djowel djowel merged commit eeca63a into cycfi:develop Sep 8, 2020
@djowel
Copy link
Member

djowel commented Sep 8, 2020

BTW, I am moving this to the raw_pixel branch. It will stay there until the quartz-2d implementation is added, so as not to break the develop branch.

@bnert
Copy link
Contributor Author

bnert commented Sep 8, 2020

Those are good points!

Not to beat a dead branch, but as far as an abstraction, do you mean something like the pixmap class in elements?

@djowel
Copy link
Member

djowel commented Sep 8, 2020

Not to beat a dead branch, but as far as an abstraction, do you mean something like the pixmap class in elements?

Not really. Something like: https://github.com/cycfi/infra/blob/master/include/infra/buffer.hpp for pixmaps. So you can write things like:

pm[3][4] = pixel;

or iterate like:

for (auto& row : pm)
   for (auto& pix : row)
      pix = some_color;

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.

2 participants