-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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 a PlatformViewRenderTarget abstraction #43813
Add a PlatformViewRenderTarget abstraction #43813
Conversation
void unlockCanvasAndPost(Canvas canvas); | ||
|
||
// The id of this render target. | ||
public long id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a style guide rule here? I see some getters prefixed with "get" and others without. I don't have a strong opinion here but it would be nice if there was a recommended approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Google Java style guide doesn't explicitly say, but many of the examples there start with get
, and my impression (maybe incorrect, I don't have a lot of Java background) is that Java convention in general was to use get
for getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// The id of this render target. | ||
public long id(); | ||
|
||
// Release backing resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Releases
(For consistency with all the other comments.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
public SurfaceTexture getTexture() { | ||
return tx; | ||
} | ||
public void setRenderTarget(@Nullable PlatformViewRenderTarget renderTarget) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a no-op? Shouldn't it change this.renderTarget
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was dead code. Removed.
665f861
to
95b31f3
Compare
- Introduce PlatformViewRenderTarget interface. - Refactor VirtualDisplayController and PlatformViewWrapper to extract SurfaceTexturePlatformViewRenderTarget into a separate class. In a future CL I will add an ImageReaderPlatformViewRenderTarget which will enable Platform Views on Impeller/Vulkan.
95b31f3
to
39d9040
Compare
…130962) flutter/engine@c902fec...2046254 2023-07-20 john@johnmccutchan.com Add a PlatformViewRenderTarget abstraction (flutter/engine#43813) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
- Introduce PlatformViewRenderTarget interface. - Refactor VirtualDisplayController and PlatformViewWrapper to extract SurfaceTexturePlatformViewRenderTarget into a separate class. In a future CL I will add an ImageReaderPlatformViewRenderTarget which will enable Platform Views on Impeller/Vulkan. Tracking issue: flutter/flutter#130892
…lutter#130962) flutter/engine@c902fec...2046254 2023-07-20 john@johnmccutchan.com Add a PlatformViewRenderTarget abstraction (flutter/engine#43813) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
In a future CL I will add an ImageReaderPlatformViewRenderTarget which will enable Platform Views on Impeller/Vulkan.
Tracking issue: flutter/flutter#130892