Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Support configurable pixel formats for the embedder API sofware rendering backend #26995

Merged
iskakaushik merged 16 commits into from
Jul 18, 2022
Merged

Support configurable pixel formats for the embedder API sofware rendering backend #26995

iskakaushik merged 16 commits into from
Jul 18, 2022

Conversation

@ardera
Copy link
Contributor

@ardera ardera commented Jun 27, 2021 •
edited
Loading

This PR adds the following things to the embedder API:

  • a new kind of backing store FlutterSoftwareBackingStore2, which is basically the same as FlutterSoftwareBackingStore but with a struct_size and pixel_format field
  • a FlutterSoftwarePixelFormat enum, containing a bunch of skia supported pixel formats, like kGray8, kRGBA8888, kRGB565
  • some tests that'll just draw solid red/blue/green into a 1x1 backing store and check that the output matches what is expected

fixes flutter/flutter#81332

One things I'm not sure of: Why is kN32_SkColorType even necessary? It just chooses BGRA8888 or RGBA8888 depending on OS (and CPU endianess I believe), could we just leave it away in this case?

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the embedder Related to the embedder API label Jun 27, 2021
@google-cla google-cla bot added the cla: yes label Jun 27, 2021
@chinmaygarde chinmaygarde self-requested a review July 1, 2021 20:17
@chinmaygarde
Copy link
Member

chinmaygarde commented Aug 12, 2021

Sorry about the late response. I was on the fence on this one. We recommend that embedder API users use the GPU backends wherever possible. The software backend has many other obvious optimizations that we have not bothered with because we only ever use it in test contexts. So, using the software backend on an already underpowered device makes me nervous. Can you elaborate your use case a bit? Is GPU backend not an option at all? Which boards are you trying to support for example? Maybe we can tinker with them ourselves.

OTOH supporting the most useful formats seems very reasonable. Can we just support popular formats like RGB 565 instead of exposing everything Skia supports?

@ardera
Copy link
Contributor Author

ardera commented Aug 12, 2021

We recommend that embedder API users use the GPU backends wherever possible. The software backend has many other obvious optimizations that we have not bothered with because we only ever use it in test contexts.

Yeah, I mean we could also look into using OpenGL via some kind of software emulation (llvmpipe, swift shader) but using the software rendering skia supports internally seemed like the most straightforward (and also least-overhead) way to go

Can you elaborate your use case a bit? Is GPU backend not an option at all? Which boards are you trying to support for example?

Sure, concretely I'm working with an i.MX 6 ULL for example. That variant of the iMX 6 has no GPU, but sadly it's still used in lots of embedded projects. It does have some hardware acceleration for common 2D graphics operations with a hardware block called the PXP, but that's exposed via a vendor-specific API and skia can't make use of it (without modification). We did some testing and it's able to do 20- maybe 25FPS, but that's with manual conversion of the graphics buffers to RGB565 using NEON SIMD stuff, so maybe 30 might be possible with direct RGB565 rendering?

30FPS (even 20 actually) is good enough, you can't really expect more with that kind of hardware. AFAICT Qt won't do more than that as well.

OTOH supporting the most useful formats seems very reasonable. Can we just support popular formats like RGB 565 instead of exposing everything Skia supports?

sure, I'll update it

@chinmaygarde
Copy link
Member

chinmaygarde commented Aug 26, 2021

Any updates on reducing the number of supported formats?

@ardera
Copy link
Contributor Author

ardera commented Sep 6, 2021

@chinmaygarde thx for the ping! forgot about it. Removed the 10bit pixel formats

@zanderso
Copy link
Member

zanderso commented Oct 7, 2021

@chinmaygarde ping.

@chinmaygarde
Copy link
Member

chinmaygarde commented Oct 14, 2021

Sorry I was on vacation for a bit and couldn't get to this patch. I think the updated format list looks good. I'll make another pass over this patch to check the updates to the public embedder API.

@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:16
@godofredoc godofredoc changed the base branch from master to main November 24, 2021 07:26
@ardera
Copy link
Contributor Author

ardera commented Dec 10, 2021

any news on this? @chinmaygarde

@zanderso zanderso requested a review from iskakaushik December 10, 2021 18:43
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jan 12, 2022

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@ardera
Copy link
Contributor Author

ardera commented Feb 8, 2022

@iskakaushik do you have some time to look at this?

@zanderso
Copy link
Member

zanderso commented Mar 31, 2022

@chinmaygarde @iskakaushik After a rebase, it looks like this should get a review.

@zanderso
Copy link
Member

zanderso commented Apr 28, 2022

From PR triage: The presubmit checks are failing due to a need to rebase.

@ardera
Copy link
Contributor Author

ardera commented Apr 30, 2022

From PR triage: The presubmit checks are failing due to a need to rebase.

thx, rebased it

iskakaushik
iskakaushik suggested changes May 2, 2022
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

left some initial comments, looks good overall

@zanderso
Copy link
Member

zanderso commented Jun 2, 2022

From PR review triage: Is this PR ready for another review pass?

@ardera
Copy link
Contributor Author

ardera commented Jun 6, 2022

@zanderso I addressed the review comments except for one, but I explained it. Maybe @iskakaushik can tell me if it's okay that way or if I should remove the last std::aborts (which can only happen inside test cases) anyway. Other than that it's ready for review again

iskakaushik
iskakaushik approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the delay here @ardera . If you fix the license issues we can land this!

@chinmaygarde
Copy link
Member

chinmaygarde commented Jun 30, 2022

@ardera Any updates on the licenses? Perhaps someone else can carry the patch to completion?

ardera added 15 commits June 30, 2022 23:32
@ardera
add sw rendering pixel format support
3a8af86 
- add new backing store type that contains struct_size and pixel format
- add new public pixel format enum with the common pixel formats supported by skia
@ardera
remove code comments
ceed493 
improve formatting
@ardera
improve documentation
038a5ea
@ardera
improve documentation
033848c
@ardera
format
fd562b8
@ardera
don't use assert
fe7fc47
@ardera
format
bcfd962
@ardera
remove some pixel formats
f5d9a43
@ardera
fix pixel format conversion
b3303b7
@ardera
empty commit to retrigger infra
02ca2fc
@ardera
add new common.h file to expected license script output
9c743b0
@ardera
fix formatting after rebase
8d0ee15
@ardera
rename common.h to pixel_formats.h
24479fc
@ardera
move some stuff, don't use abort
a3da3b1 
- move getSkColorType, getSkColorInfo to pixel_formats.cc
- make them return optional so we can indicate an error
- don't use abort there
@ardera
fix formatting
293ad69
@ardera ardera force-pushed the master branch 2 times, most recently from cf8b1dd to 293ad69 Compare June 30, 2022 21:49
@skia-gold
Copy link

skia-gold commented Jun 30, 2022

Gold has detected about 12 new digest(s) on patchset 16.
View them at https://flutter-engine-gold.skia.org/cl/github/26995

@ardera
Copy link
Contributor Author

ardera commented Jun 30, 2022

@chinmaygarde licenses should be fixed now. though don't know what that gold bot comment is about

@chinmaygarde chinmaygarde added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jul 14, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 14, 2022

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 14, 2022

Validations Fail.

@iskakaushik iskakaushik merged commit 8286098 into flutter:main Jul 18, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2022
@engine-flutter-autoroll
8286098 Support configurable pixel formats for the embedder API sofwa…
90eaab3 
…re rendering backend (flutter/engine#26995)
@ardera ardera mentioned this pull request Jan 10, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde chinmaygarde is a code owner

1 more reviewer

@iskakaushik iskakaushik iskakaushik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes embedder Related to the embedder API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow specifying the pixel format for the embedder API software rendering backend

5 participants

@ardera @chinmaygarde @zanderso @skia-gold @iskakaushik