Skip to content
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 OCOS_ENABLE_VENDOR_IMAGE_CODECS compile definition, even if VISION is disabled #849

Closed
wants to merge 2 commits into from

Conversation

stwykd
Copy link

@stwykd stwykd commented Nov 21, 2024

  • As per PR#808, I shouldn't need libpng and libjpeg on Windows and Apple, if OCOS_ENABLE_VENDOR_IMAGE_CODECS is enabled
  • The CMakeLists.txt only adds the OCOS_ENABLE_VENDOR_IMAGE_CODECS compile definition when OCOS_ENABLE_VISION is set, assuming that decoders are not used if VISION is not enabled. That's not the case because shared\api\image_processor.cc is added for OCOS_ENABLE_C_API (CMakeLists.txt:761) and OCOS_BUILD_SHARED_LIB=OFF (CMakeLists.txt:873)

In this change:

  • Handling for -DOCOS_ENABLE_VENDOR_IMAGE_CODECS is performed even if VISION is not enabled.
  • I removed _DEFAULT_CODEC_ENABLE, and if(_DEFAULT_CODEC_ENABLE) as it is redundant given it's preceded by set(_DEFAULT_CODEC_ENABLE ON)

@stwykd stwykd requested a review from a team as a code owner November 21, 2024 10:13
@stwykd
Copy link
Author

stwykd commented Nov 21, 2024

@microsoft-github-policy-service agree company="Microsoft"

@wenbingl
Copy link
Member

/azp run "onnxruntime-extensions.CI"

Copy link

No pipelines are associated with this pull request.

@skyline75489
Copy link
Collaborator

skyline75489 commented Nov 22, 2024

Would this conflict with #848 ?

No, it actually depends on #848 , by which we turn on the DOCOS_ENABLE_VENDOR_IMAGE_CODECS by default.

@wenbingl
Copy link
Member

/azp run onnxruntime-extensions.CI

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wenbingl
Copy link
Member

  • As per PR#808, I shouldn't need libpng and libjpeg on Windows and Apple, if OCOS_ENABLE_VENDOR_IMAGE_CODECS is enabled
  • The CMakeLists.txt only adds the OCOS_ENABLE_VENDOR_IMAGE_CODECS compile definition when OCOS_ENABLE_VISION is set, assuming that decoders are not used if VISION is not enabled. That's not the case because shared\api\image_processor.cc is added for OCOS_ENABLE_C_API (CMakeLists.txt:761) and OCOS_BUILD_SHARED_LIB=OFF (CMakeLists.txt:873)

In this change:

  • Handling for -DOCOS_ENABLE_VENDOR_IMAGE_CODECS is performed even if VISION is not enabled.
  • I removed _DEFAULT_CODEC_ENABLE, and if(_DEFAULT_CODEC_ENABLE) as it is redundant given it's preceded by set(_DEFAULT_CODEC_ENABLE ON)

_DEFAULT_CODEC_ENABLE was added for these platforms which doesn't have native APIs for image codecs so that we don't need explicitly turn this flag ON or OFF for pipelines on different platforms. If this PR can resolve issue. we can merge this PR to unblock you since this flag was OFF now so it might not break any pipeline.

Moreover, OCOS_ENABLE_VENDOR_IMAGE_CODECS cannot be ON by default is due to missing encoder support, with #848, we will turn on this flag.

@stwykd
Copy link
Author

stwykd commented Nov 22, 2024

ok. so #848 will turn on OCOS_ENABLE_VENDOR_IMAGE_CODECS by default. I can wait for that PR

@stwykd stwykd closed this Nov 22, 2024
@skyline75489
Copy link
Collaborator

@stwykd #855 will be the attempt to turn on OCOS_ENABLE_VENDOR_IMAGE_CODECS by default, in case you're interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants