embedder: fix bit-order in software pixel format description

[ardera]

The order of the components for packed software pixel formats is incorrectly documented as being the order in the native type, least-significant-bit first. In reality it's the other way around. For example, for RGB565, the R is the 5 most significant bits in the 2-byte pixel value, rather than the least significant bits. The test even verify it is that way:

https://github.com/flutter/engine/blob/main/shell/platform/embedder/tests/embedder_unittests.cc#L2782-L2785

I assume noone used the software pixel formats until @sodiboo did, that's why it's gone unnoticed for so long.

Also contains some other minor documentation improvements.

• Issue: [embedder] Software Pixel Formats, Compositor documentation issue flutter#160149

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 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.

[sodiboo]

is the distinction between "array formats" and "packed formats" necessarily meaningful now? because it seems to me that, all of them can be interpreted as being packed into a single integer (8-, 16-, or 32-bit), with the first channel being the most significant bits, and the last channel being the least significant ones, and then.... the 8888 formats are encoded as little endian, and everything else native endian??

like yes, the array-like accessing is more convenient but uhh, is it really correct to say that's what it's supposed to mean? aren't they all "packed", even if 8888 is also "aligned"?

also, is it actually true that those formats are specifically fixed-little endian and the others are native endian? if so, then wow, cursed.

thinking about pixel formats makes my head hurt :(

[sodiboo]

If this is native endian, then it will only match DRM formats on little endian systems because they are always little endian. according to this resource you linked on discord.

Also, given that this is for the software rendering backend, maybe it makes more sense to reference the wl_shm formats? since wayland-drm is for EGL. Both are identical, I think? wl_shm formats are explicitly also based on Linux DRM codes and explicitly little endian. Maybe reference DRM codes instead of any Wayland protocol?

Suggested change

	/// This is equivalent to wayland format RGB565 (WL_DRM_FORMAT_RGB565).
	/// On little endian systems, this is equivalent to wayland format RGB565 (WL_SHM_FORMAT_RGB565).

[ardera]

If this is native endian, then it will only match DRM formats on little endian systems because they are always little endian. according to this resource you linked on discord.

True; though I'd also provide some context and say that big endian is super rare and in the vast majority of cases this is equivalent, for the lucky people that didn't have to think about endianess until now.

Also, given that this is for the software rendering backend, maybe it makes more sense to reference the wl_shm formats? since wayland-drm is for EGL. Both are identical, I think? wl_shm formats are explicitly also based on Linux DRM codes and explicitly little endian. Maybe reference DRM codes instead of any Wayland protocol?

Yes AFAICT they're both the based on the DRM codes. Though, I think still better to leave no room for confusion and give the precise wayland formats (maybe both WL_DRM_... and WL_SHM_...). I think if you're dealing with DRM directly you might know that WL_DRM_FORMAT_... also equals DRM_FORMAT_...

[ardera]

is the distinction between "array formats" and "packed formats" necessarily meaningful now? because it seems to me that, all of them can be interpreted as being packed into a single integer (8-, 16-, or 32-bit), with the first channel being the most significant bits, and the last channel being the least significant ones, and then.... the 8888 formats are encoded as little endian, and everything else native endian??

Well yes, I think you can remove that distinction and say both are native types and then differentiate between endianess, though you still have to rename the current array formats, right?
If you assume BGRA8888 is a packed format, on little-endian, with A being the least significant bits, it would be at the lowest memory address. But right now, as an array format, it's the highest memory address. So it'd have to be renamed ARGB8888.

I think there is a point for following the linux naming, but on the other hand, it's just a fact that there's different conventions. Maybe Windows matches more closely with the current naming, and changing it would make the API less usable for Windows users, idk. You just have to follow some convention, and in this case I think it's most natural to just follow the Skia convention.

Also in general, distinguishing between endianess is purely a theoretical issue since I'm not sure Flutter, Skia and much less Dart even build/work on big-endian.

like yes, the array-like accessing is more convenient but uhh, is it really correct to say that's what it's supposed to mean? aren't they all "packed", even if 8888 is also "aligned"?

also, is it actually true that those formats are specifically fixed-little endian and the others are native endian? if so, then wow, cursed.

I don't know what Skia does internally, but the only reference I could find says this is how it works. Might be because that's how it's implemented for software rendering.

[sodiboo]

Well yes, I think you can remove that distinction and say both are native types and then differentiate between endianess, though you still have to rename the current array formats, right?
If you assume BGRA8888 is a packed format, on little-endian, with A being the least significant bits, it would be at the lowest memory address. But right now, as an array format, it's the highest memory address. So it'd have to be renamed ARGB8888.

o. yea. forgot about that. in my mind, Bgra888 == default Wayland wl_shm format == duh of course it's "little endian packed".

yes, the naming would indeed have to change for the lack of distinction to be helpful, and indeed it makes no sense to do so.

[ardera]

o. yea. forgot about that. in my mind, Bgra888 == default Wayland wl_shm format == duh of course it's "little endian packed".

yeah, well, I got it wrong too 😅

btw you mentioned some other stuff on discord, if there's some other documentation that can be improved let me know and I can add that to the PR

[ardera]

Well, seems like someone else was quicker: #57210
lmk if it's still worth merging for the other minor clarifications

[cbracken]

Apologies for missing this earlier. Was catching up on GitHub issues in reverse chronological order and got to the newer patch first.

Thanks for the additional clarifying doc comments; these lgtm!

[auto-submit]

auto label is removed for flutter/engine/57156, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[jonahwilliams]

LGTM

[ardera]

Apologies for missing this earlier. Was catching up on GitHub issues in reverse chronological order and got to the newer patch first.

Thanks for the additional clarifying doc comments; these lgtm!

all good, thanks!