Ios nv12 external texture by cikelengfeng · Pull Request #20082 · flutter/engine · GitHub

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

Contributor

cikelengfeng commented Jul 28, 2020

iOS external texture supports rendering NV12 pixelbuffer

At present, the external texture of iOS only supports pixelbuffer in BGRA32 format, but many times we need to use pixelbuffer in YUV format, such as NV12. Compared with BGRA32, NV12 requires less memory (about 37.5% of BGRA32), so we need to support rendering pixelbuffer in NV12 format.

Related Issues

flutter/flutter#31041

Tests

Because I am modifying the existing code, I need to add test cases to the existing unit tests of ios_external_texture_gl and ios_external_texture_metal, but I did not find these unit tests. Could anyone tell me where they are?

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard bot added the platform-ios label

flutter-dashboard bot commented Jul 28, 2020

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

googlebot commented Jul 28, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment

googlebot commented Jul 28, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot added the cla: no label

auto-assign bot requested a review from gaaclarke

July 28, 2020 13:12

cikelengfeng force-pushed the ios_nv12_external_texture branch from c36edc5 to 917b4fa Compare

July 28, 2020 13:18

googlebot commented Jul 28, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment

googlebot commented Jul 28, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added cla: yes and removed cla: no labels

googlebot commented Jul 29, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment

googlebot commented Jul 29, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

cikelengfeng force-pushed the ios_nv12_external_texture branch from 5cf1479 to 774e383 Compare

July 29, 2020 04:29

googlebot commented Jul 29, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment

googlebot commented Jul 29, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added cla: yes and removed cla: no labels

cikelengfeng force-pushed the ios_nv12_external_texture branch from da0f59e to 3306b72 Compare

July 29, 2020 08:51

Contributor Author

cikelengfeng commented Jul 30, 2020

@gaaclarke Could you help me review this pr?

gaaclarke reviewed

View reviewed changes

Member

gaaclarke left a comment

Thanks @cikelengfeng I'm not sure about automated testing for this @chinmaygarde may have some ideas. Do you have code that that exercises this that can be run manually?

The code is failing format checks, can you please address all of the CI issues as well.

shell/platform/darwin/ios/ios_external_texture_gl.mm Outdated

    
                SkISize size{yBackendTexture.width(), yBackendTexture.height()};

                sk_sp<SkImage> image =

                    SkImage::MakeFromYUVATextures(context, kRec601_SkYUVColorSpace, nv12TextureHandles,

                                                  yuvaIndices, size, kTopLeft_GrSurfaceOrigin, nullptr);

Member

gaaclarke Jul 30, 2020

nit: nullptr argument would benefit from an argument name comment

Contributor Author

cikelengfeng Jul 31, 2020 •

edited

Loading

This is to keep the style consistent with the original code. Do I need to modify the original code according to your suggestions?

Member

gaaclarke Jul 31, 2020

Yep, https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments

Contributor Author

cikelengfeng Aug 3, 2020

done

shell/platform/darwin/ios/ios_external_texture_gl.mm Outdated

    
                return image;

              }

              bool IOSExternalTextureGL::IsTexturesAvailable() {

Member

gaaclarke Jul 30, 2020

This should be a const method.

Contributor Author

cikelengfeng Jul 31, 2020

done

shell/platform/darwin/ios/ios_external_texture_gl.mm Outdated Show resolved Hide resolved

shell/platform/darwin/ios/ios_external_texture_gl.h Outdated

    
                fml::CFRef<CVOpenGLESTextureCacheRef> cache_ref_;

                fml::CFRef<CVOpenGLESTextureRef> texture_ref_;

                fml::CFRef<CVPixelBufferRef> buffer_ref_;

                OSType pixelFormat = 0;

Member

gaaclarke Jul 30, 2020

These variables don't adhere to the google c++ style guide, use pixel_format_. Same goes for down below.

Contributor Author

cikelengfeng Jul 31, 2020

done.

shell/platform/darwin/ios/ios_external_texture_metal.mm Outdated

    
                CVMetalTextureRef y_metal_texture_raw = NULL;

                {

                  auto cv_return =

                      CVMetalTextureCacheCreateTextureFromImage(kCFAllocatorDefault,    // allocator

Member

gaaclarke Jul 30, 2020

Please use checked argument name comments (these are checked by the linter):

CVMetalTextureCacheCreateTextureFromImage(/*allocator=*/ kCFAllocatorDefault,

Contributor Author

cikelengfeng Jul 31, 2020

This is also to keep the style consistent with the original code. Do I need to modify the original code according to your suggestions?

Member

gaaclarke Jul 31, 2020

https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments

Contributor Author

cikelengfeng Aug 3, 2020

done

shell/platform/darwin/ios/ios_external_texture_metal.mm Outdated

    
                      CVMetalTextureCacheCreateTextureFromImage(kCFAllocatorDefault,    // allocator

                                                                texture_cache_,         // texture cache

                                                                pixel_buffer,           // source image

                                                                NULL,                   // texture attributes

Member

gaaclarke Jul 30, 2020

s/NULL/nullptr

Contributor Author

cikelengfeng Jul 31, 2020

Since NULL was used in the original code, in order to keep the style consistent, I also used NULL, do I need to change all to nullptr?

Member

gaaclarke Jul 31, 2020

https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

Contributor Author

cikelengfeng Aug 3, 2020

done

shell/platform/darwin/ios/ios_external_texture_metal.mm

    
                    SkYUVAIndex{1, SkColorChannel::kR},  // Read U data from the red channel of the second texture

                    SkYUVAIndex{1,

                                SkColorChannel::kG},  // Read V data from the green channel of the second texture

                    SkYUVAIndex{-1, SkColorChannel::kA}};  //-1 means to omit the alpha data of YUVA

Member

gaaclarke Jul 30, 2020

If we are omitting the alpha data, why is this method referring to yuva?

Contributor Author

cikelengfeng Jul 31, 2020

Because some YUV formats have alpha channels (such as AYUV), skia may need to be compatible with such formats. In addition, I refer to the implementation of SkImage::MakeFromNV12TexturesCopy.

Contributor Author

cikelengfeng Jul 31, 2020

And NV12 has no alpha channel.

Member

gaaclarke Jul 31, 2020

Should we change the name of this method then? It's called WrapYUVAExternalPixelBuffer.

Contributor Author

cikelengfeng Jul 31, 2020

good idea，it should be WrapNV12ExternalPixelBuffer

Contributor Author

cikelengfeng Aug 3, 2020

done

shell/platform/darwin/ios/ios_external_texture_metal.mm Show resolved Hide resolved

Contributor Author

cikelengfeng commented Jul 31, 2020 •

edited

Loading

Thanks @cikelengfeng I'm not sure about automated testing for this @chinmaygarde may have some ideas. Do you have code that that exercises this that can be run manually?

The code is failing format checks, can you please address all of the CI issues as well.

This is the Flutter camera plugin. I modified the code to support the output of NV12 pixelbuffer. The engine compiled with this pr can run. You may view the memory usage through the instruments tool at runtime.
camera.zip
1920x1080 resolution BGRA32 memory information:

1920x1080 resolution NV12 memory information:

And the failed checks only output Step('format and dart test') (retcode: 1). How should I solve this problem?

cikelengfeng force-pushed the ios_nv12_external_texture branch from 3306b72 to 6112658 Compare

July 31, 2020 06:32

Member

gaaclarke commented Jul 31, 2020

This sounds good to me. I just have a few style nits left. I imagine there is a performance gain too since the camera is probably using YUV natively. The GPU was probably performing a pixel conversion to get ABGR or whatever.

Can you post a PR to the camera plugin that takes advantage of this work and link it here? You will probably have to have it disabled for now until this change lands on stable. If we can't get tests for it, we should at least have that. Let's wait to see if chinmay has any better idea about automated testing.


          iOS external texture supports rendering NV12 pixelbuffer

6cb5d6c

cikelengfeng force-pushed the ios_nv12_external_texture branch from 6112658 to 6cb5d6c Compare

August 3, 2020 03:37

cikelengfeng mentioned this pull request

ios camera takes advantage of NV12 pixelbuffer flutter/plugins#2905

Closed

13 tasks

Contributor Author

cikelengfeng commented Aug 3, 2020

This sounds good to me. I just have a few style nits left. I imagine there is a performance gain too since the camera is probably using YUV natively. The GPU was probably performing a pixel conversion to get ABGR or whatever.

Can you post a PR to the camera plugin that takes advantage of this work and link it here? You will probably have to have it disabled for now until this change lands on stable. If we can't get tests for it, we should at least have that. Let's wait to see if chinmay has any better idea about automated testing.

Yes, but I don’t think we need to worry. GPU needs to use a shader to convert YUV to ABGR. This is what GPU is good at. It is very fast.
The PR of the camera is here.

gaaclarke approved these changes

View reviewed changes

Member

gaaclarke left a comment

I manually tested the PR to make sure existing behavior doesn't break on iOS. I don't have a good idea how to automate tests for this but the OP was able to provide steps to manually test it with their PR to the camera plugin. LGTM.

gaaclarke merged commit fa16c3d into flutter:master

engine-flutter-autoroll mentioned this pull request

Roll Engine from 02d71d60effa to fa16c3d0bacd (3 revisions) flutter/flutter#62813

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          fa16c3d iOS external texture supports rendering NV12 pixelbuffer (flu…

2efaf03

…tter/engine#20082)

befovy commented Oct 17, 2020 •

edited

Loading

Hi, anyone can help me? How to dynamically check if engien support nv12 texture in a flutter video player plugin?

Contributor Author

cikelengfeng commented Oct 19, 2020

Hi, anyone can help me? How to dynamically check if engien support nv12 texture in a flutter video player plugin?

It is not yet possible to dynamically check, can you talk about your usage scenario in detail?

befovy commented Oct 24, 2020

Hi, anyone can help me? How to dynamically check if engien support nv12 texture in a flutter video player plugin?

It is not yet possible to dynamically check, can you talk about your usage scenario in detail?

Hi， I developed a flutter video player https://github.com/befovy/fijkplayer. It use flutter engine texture to display video.
Currently, I return a bgra pixelbuffer in https://github.com/befovy/fijkplayer/blob/master/ios/Classes/FijkPlayer.m#L243.

I want to upgrade fijkplayer then return yuv pixelbuffer to flutter engine, but I also want fijkplayer works in old flutter version.

Contributor Author

cikelengfeng commented Oct 26, 2020

Hi, anyone can help me? How to dynamically check if engien support nv12 texture in a flutter video player plugin?

It is not yet possible to dynamically check, can you talk about your usage scenario in detail?

Hi， I developed a flutter video player https://github.com/befovy/fijkplayer. It use flutter engine texture to display video.
Currently, I return a bgra pixelbuffer in https://github.com/befovy/fijkplayer/blob/master/ios/Classes/FijkPlayer.m#L243.

I want to upgrade fijkplayer then return yuv pixelbuffer to flutter engine, but I also want fijkplayer works in old flutter version.

I'm afraid we can't do it, because even if you add a query interface to the latest version, if the user uses a version that supports NV12 but does not have this interface (such as 1.22), your code will still produce incorrect behavior.
I suggest that you may create another version of your library and specify SDK constraints in the pubspec file of this version, such as:

environment:
  sdk: '>=2.1.0 <3.0.0'
  flutter: ^1.22.0

see: https://dart.dev/tools/pub/pubspec#sdk-constraints

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-ios