[NOT FOR MERGING] iOS Image Embedder Objective C++ #830
Conversation
priankakariat
commented
Jun 3, 2022
priankakariat
commented
- Added a test .mm file and corresponding test for Image Embedder
- Added a new .sh file for building framework archive
- Added new podspec for objective c++ frameworks
| @@ -0,0 +1,20 @@ | |||
| Pod::Spec.new do |s| | |||
| s.name = 'TensorFlowLiteTaskVisionObjCPP' | |||
There was a problem hiding this comment.
Instead of having a dedicated podspec for the ObjC++ implementation, can you add the TensorFlowLiteTaskVisionObjCPP.framework as a vendored framework for the current TensorFlowLiteTaskVision pod? We don't want to expose the internal structure of the framework to the end-users if possible.
There was a problem hiding this comment.
This was just meant for illustrating how it will work. I have added the entire vision framework as a vendored framework now for more clarity. You can refer to the new vision podspec. The vendor framework mentioned here is defined in this build file.
We can also build the text framework in a similar manner going forward. I haven't made that change. Once it is done, the two shell scripts in /tensorflow_lite_support/tools/ci_build/builds can be unified into one.
There was a problem hiding this comment.
Thanks Prianka! LGTM.
@schmidt-sebastian @lu-wang-g Do you have any feedback?
There was a problem hiding this comment.
@priankakariatyml Can you check if the new setup can make the Vision pod and Text pod co-exist in the same project? I guess that you may tweak the code to hide the duplicating symbols.
khanhlvg
changed the title
tensorflow_lite_support/ios/TensorFlowLiteTaskVision.podspec.template
Outdated
Show resolved
Hide resolved
| } | ||
| else { |
There was a problem hiding this comment.
| } | |
| else { | |
| } else { |
There was a problem hiding this comment.
Looks good to me at a high level.
As far as I can tell, this approach would work because the ObjC API headers (e.g., TFLImageEmbedder.h) are only using the other ObjC API headers in the same directory and not including any other C++ headers directly.
As noted, the header prefix stripping macro is probably not needed, but I'm not 100% sure about that. Please double check.
| # to the "Headers" directory with no header path prefixes. This auxiliary rule | ||
| # is used for stripping the path prefix from the iOS API header files imported by | ||
| # other iOS API header files. | ||
| def strip_ios_api_include_path_prefix(name, hdr_labels, prefix = ""): |
There was a problem hiding this comment.
- Do we actually need this new macro?
As far as I can tell, the new iOS header files (e.g., TFLImageEmbedder.h) are all using plain header filenames in their import statements. That is, it's likely that the processed header file will be the same as the original header file.
- If you actually need it (which is probably not the case in my understanding..), does this actually need to be separated from the
strip_c_api_include_path_prefixhelper above?
There was a problem hiding this comment.
- Yes stripping will be needed. Right now only one file has been added for reviewing the structure. In a full implementation the header would have a lot of imports in a hierarchical structure like TFLImageClassifier.h. We will have to strip the prefixes and build the final iOS framework like the C framework is built.
I added a separate method since objective c files are imported using#importas opposed to c files which are included using#include. - I can incorporate this check in
strip_c_api_include_path_prefixand call itstrip_api_include_path_prefixwhich will strip path prefixes irrespective of#importor#include
There was a problem hiding this comment.
I see. If it's needed, I'd recommend consolidating these two functions into a single one as mentioned before.
| s.library = 'c++' | ||
| s.vendored_frameworks = 'Frameworks/TensorFlowLiteTaskVisionC.framework' | ||
| s.frameworks = 'CoreMedia', 'Accelerate' | ||
| s.vendored_frameworks = 'Frameworks/TensorFlowLiteTaskVisionObjCPP.framework' |
There was a problem hiding this comment.
Shouldn't this be identical to the "bundle" name of the ios_static_framework rule?
There was a problem hiding this comment.
Sorry, my bad. While testing the pod I had named the bundle TensorFlowLiteTaskVisionObjCPP. Afterwards I changed it to TensorFlowLiteTaskVision but didn't change it here. I have updated it now.
| deps = [ | ||
| "//tensorflow_lite_support/cc/task/vision:image_embedder", | ||
| ], | ||
| copts = ["-ObjC++","-std=c++14"], |
There was a problem hiding this comment.
Why is the -ObjC++ option needed?
There was a problem hiding this comment.
My understanding was that "-ObjC++" is needed when you are building with .mm files. Our source file is .mm.
