properly namespace flutter software pixel formats

[ardera]

Renames the FlutterSoftwarePixelFormat enum values of flutter_embedder.h:

kGray8 --> kFlutterSoftwarePixelFormatGray8
kRGB565 --> kFlutterSoftwarePixelFormatRGB565
...

Fixes flutter/flutter#118247.

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.

[ardera]

Some questions:

1. Do the refactors in embedder_unittests.cc satisfy the I added tests checkmark above? Or should I ask for an explicit test exemption (or add tests somehow)?
2. I think the fork-point for the next stable has already happened, right? Could this be cherry-picked into whatever branch stable will be based on, so the change makes it into next stable? Otherwise this PR is pointless

[chinmaygarde]

This is a breaking API change to the one public header with stability guarantees.

[ardera]

@chinmaygarde I thought it was only ABI stability guarantees, not API stability guarantees?

The info at the top explicitly states renaming enum members is allowed, as long as the associated values don't change:

engine/shell/platform/embedder/embedder.h

Lines 29 to 34 in 2c534dc

	// These changes are allowed:
	// - Adding new struct members at the end of a structure.
	// - Adding new enum members with a new value.
	// - Renaming a struct member as long as its type, size, and intent remain the
	// same.
	// - Renaming an enum member as long as its value and intent remains the same.

And I think API breakage is less of a concern in this case, since this is a fairly niche API which isn't even present on stable yet.

[chinmaygarde]

I thought it was only ABI stability guarantees, not API stability guarantees?

We still try to minimize them as much as possible. There are other instances where we could change things if we could. Like kSuccess for instance. I remember changing just these fields to be deprecated caused a whole bunch of failures. We could have instead aborted engine launch with an error message or added compiler warnings about the deprecation.

And I think API breakage is less of a concern in this case, since this is a fairly niche API which isn't even present on stable yet.

I think this is compelling though. I'm LGTM-ing based on this but I am also not sure who all are affected. I couldn't find users of this in the locations I know of. If you know if any, can you ping them if able? Just so we minimize the disruption.

If you think the disruption is manageable, go for it. If not, I trust you to hold 🙏🏽

[ardera]

We still try to minimize them as much as possible. There are other instances where we could change things if we could. Like kSuccess for instance. I remember changing just these fields to be deprecated caused a whole bunch of failures. We could have instead aborted engine launch with an error message or added compiler warnings about the deprecation.

Makes sense. Yeah kSuccess also seems a bit non-unique. Maybe, since this mistake has happened two times now, it's worth adding some a note to the embedder.h documentation that says "use only unique (flutter namespaced) identifiers"?

I think this is compelling though. I'm LGTM-ing based on this but I am also not sure who all are affected. I couldn't find users of this in the locations I know of. If you know if any, can you ping them if able? Just so we minimize the disruption.

If you think the disruption is manageable, go for it. If not, I trust you to hold 🙏🏽

I only introduced this change because we had an iMX6 lying around that had no GPU and could only scanout RGB565, and even we haven't used this API yet :) I don't know any other embedders that do. Generally, no common embedders seem to use / support flutters' software rendering backend.

I think in this case the disruption is manageable. Would it be possible to cherry-pick this change into whatever branch becomes the next stable? (If the forkpoint has already happened) Otherwise, if the next stable releases with the old kRGB565 enum names, it may be better to leave them that way.

[ardera]

Would it be possible to cherry-pick this change into whatever branch becomes the next stable? (If the forkpoint has already happened)

Ah, I guess not 😄

Maybe it's better to choose a different approach now, perhaps something like this:

typedef enum {
  kFlutterSoftwarePixelFormatGray8,
  kFlutterSoftwarePixelFormatRGB565,
  // ...
#ifdef FLUTTER_DEPRECATED_SOFTWARE_PIXEL_FORMAT_NAMES
  kGray8 = kFlutterSoftwarePixelFormatGray8,
  kRGB565 = kFlutterSoftwarePixelFormatRGB565,
  // ...
#endif
} FlutterSoftwarePixelFormat;

People will still get a build breakage if they happen to use the old names, but at least it's easy to fix (just define FLUTTER_DEPRECATED_SOFTWARE_PIXEL_FORMAT_NAMES) At least that's in theory, in practice I could imagine noone will get a build breakage at all, because this is (again) pretty niche API and noone really seems to use the software backend

[chinmaygarde]

At least that's in theory, in practice I could imagine noone will get a build breakage at all, because this is (again) pretty niche API and noone really seems to use the software backend

In that case, we should just land it.

[jonahwilliams]

RSLGTM