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

[libjpeg-turbo] Update to libjpeg-turbo 2.0.1 #4635

Merged
merged 7 commits into from
May 6, 2019

Conversation

UnaNancyOwen
Copy link
Contributor

Update libjpeg-turbo port to libjpeg-turbo 2.0.0.
It contains bug fixes and performance improvements.

@alexkaratarakis
Copy link
Contributor

Similar to #4005 but there are diffs.

Update libjpeg-turbo port to libjpeg-turbo 2.0.0.
@UnaNancyOwen
Copy link
Contributor Author

UnaNancyOwen commented Nov 2, 2018

@alexkaratarakis Thank you for the information. I was read earlier sent pull request #4005. But, It contains some mistakes. It will install turbojpeg, but optimized libjpeg will not be installed. It is bug of patch.

Should I feedback to earlier sent pull request? What do you think?
(However, It seems that pull request has not been discussed for a while.)

@alexkaratarakis
Copy link
Contributor

@UnaNancyOwen It has been a while from the previous PR, so I think we should keep the discussions here.

Currently running it through CI, will update with the results! :)

@alexkaratarakis
Copy link
Contributor

There are regressions for libjpeg-turbo itself as well as freeimage and vxl. Doesn't look too bad :)

  • libjpeg-turbo:arm-uwp
  • libjpeg-turbo:x64-linux
  • libjpeg-turbo:x64-uwp
  • freeimage:x64-windows
  • freeimage:x64-windows-static
  • freeimage:x86-windows
  • vxl:x64-windows-static
  • vxl:x86-windows

@UnaNancyOwen
Copy link
Contributor Author

UnaNancyOwen commented Nov 4, 2018

In the case of UWP (x64-uwp, arm-uwp), CMAKE_SYSTEM_PROCESSOR seems to be empty.
Therefore, An error occurs in simd/CMakeLists.txt#L372, because the CPU architecture is not set to CPU_TYPE in CMakeLists.txt#L41-L68.
In order to solve this problem, I have two ideas.

  • Add CMake option -DCMAKE_SYSTEM_PROCESSOR={x86|arm} in libjpeg-turbo/portfile.cmake.

  • Implement triplet variable VCPKG_CMAKE_SYSTEM_PROCESSOR to vcpkg. (like VCPKG_CMAKE_SYSTEM_NAME, VCPKG_CMAKE_SYSTEM_VERSION)

@UnaNancyOwen
Copy link
Contributor Author

@alexkaratarakis @ras0219-msft What do you think?

Fix performing post-build validation on linux by remove man direcotry and files.
@UnaNancyOwen
Copy link
Contributor Author

UnaNancyOwen commented Nov 9, 2018

(It may be different from the actual machine because I tried on virtual machine,) In the case of Linux (x64-linux), CMAKE_SYSTEM_PROCESSOR seems to be empty such as case of UWP.
The problem about CMAKE_SYSTEM_PROCESSOR is not problem of this port alone.
I think it should be discussed and fix with another issues or pull request.

@UnaNancyOwen
Copy link
Contributor Author

If you don't find other problem, I think ready to merge this pull request.

@UnaNancyOwen
Copy link
Contributor Author

UnaNancyOwen commented Nov 10, 2018

  • freeimage:x64-windows
  • freeimage:x64-windows-static
  • freeimage:x86-windows

In case of Debug configuration, find_package(JPEG) can find debug/lib/jpeg.lib, but it is include in release variable (JPEG_LIBRARY_RELEASE).

  • Debug
-- Found JPEG: C:/vcpkg/installed/x64-windows/debug/lib/jpeg.lib (found version "62") 
-- --------------------------------------
-- JPEG_INCLUDE_DIR : C:/vcpkg/installed/x64-windows/include
-- JPEG_LIBRARY_RELEASE : C:/vcpkg/installed/x64-windows/debug/lib/jpeg.lib
-- JPEG_LIBRARY_DEBUG : JPEG_LIBRARY_DEBUG-NOTFOUND
-- --------------------------------------
  • Release
-- Found JPEG: C:/vcpkg/installed/x64-windows/lib/jpeg.lib (found version "62") 
-- --------------------------------------
-- JPEG_INCLUDE_DIR : C:/vcpkg/installed/x64-windows/include
-- JPEG_LIBRARY_RELEASE : C:/vcpkg/installed/x64-windows/lib/jpeg.lib
-- JPEG_LIBRARY_DEBUG : JPEG_LIBRARY_DEBUG-NOTFOUND
-- --------------------------------------

In addition, libjpeg-turbo 2.0.0 includes two additional functions. (jpeg_read_icc_profile() and jpeg_write_icc_profile()) Please read this release note.
Therefor, It is occurs error C2556 about override, because jpeg_read_icc_profile() is already defined in PluginJPEG.cpp of freeimage.

C:\vcpkg\buildtrees\freeimage\src\FreeImage\Source\FreeImage\PluginJPEG.cpp(506): error C2556: 'BOOL jpeg_read_icc_profile(j_decompress_ptr,JOCTET **,unsigned int *)': overloaded function differs only by return type from 'boolean jpeg_read_icc_profile(j_decompress_ptr,JOCTET **,unsigned int *)'
C:\vcpkg\installed\x64-windows\include\jpeglib.h(1066): note: see declaration of 'jpeg_read_icc_profile'
C:\vcpkg\buildtrees\freeimage\src\FreeImage\Source\FreeImage\PluginJPEG.cpp(506): error C2371: 'jpeg_read_icc_profile': redefinition; different basic types
C:\vcpkg\installed\x64-windows\include\jpeglib.h(1066): note: see declaration of 'jpeg_read_icc_profile'

@UnaNancyOwen
Copy link
Contributor Author

UnaNancyOwen commented Nov 11, 2018

  • freeimage:x64-windows
  • freeimage:x64-windows-static
  • freeimage:x86-windows

@alexkaratarakis I fixed these in #4703. Please review it.

@UnaNancyOwen
Copy link
Contributor Author

  • libjpeg-turbo:arm-uwp
  • libjpeg-turbo:x64-linux
  • libjpeg-turbo:x64-uwp

@alexkaratarakis I fixed these in this pull request and #4688. Please review it.

@UnaNancyOwen
Copy link
Contributor Author

  • vxl:x64-windows-static
  • vxl:x86-windows

@alexkaratarakis I fixed these in #4711. Please review it. (It does not seem to be related to this change.)

@UnaNancyOwen
Copy link
Contributor Author

It seems that all problems has been solved! 👍

@alexkaratarakis alexkaratarakis self-assigned this Nov 13, 2018
@alexkaratarakis
Copy link
Contributor

Note: Only #4688 remaining, everything else is merged.

Fix issue that fails rename when VCPKG_BUILD_TYPE is set.
@UnaNancyOwen
Copy link
Contributor Author

Fix #4754

@lhecker
Copy link
Member

lhecker commented Nov 27, 2018

@UnaNancyOwen libjpeg-turbo 2.0.1 was released 2 weeks ago and contains some few very significant bug fixes.
IMHO it might be beneficial to upgrade to 2.0.1 before we merge this PR.

Furthermore in my limited testing I'd like to note that WITH_SIMD should now work on all x86/x86_64 platforms and should IMO be enabled everywhere instead of just for WindowsStore.
In my experience that e.g. boosts JPEG decode performance by 4x.

Update libjpeg-turbo port to libjpeg-turbo 2.0.1.
@UnaNancyOwen UnaNancyOwen changed the title [libjpeg-turbo] Update to libjpeg-turbo 2.0.0 [libjpeg-turbo] Update to libjpeg-turbo 2.0.1 Nov 27, 2018
@UnaNancyOwen
Copy link
Contributor Author

@lhecker Thanks! 👍

@UnaNancyOwen
Copy link
Contributor Author

@alexkaratarakis @ras0219-msft I think ready to merge.

@Rastaban Rastaban self-assigned this Mar 14, 2019
@Rastaban
Copy link
Contributor

Resolved merge conflicts and running CI tests.

@Rastaban
Copy link
Contributor

I am seeing the following regressions:

Processing arm-uwp                        409 vs 415
     libjpeg-turbo                 **regression**                Fail vs Pass
 Processing x64-linux                      605 vs 635
     libjpeg-turbo                 **regression**                Fail vs Pass
 Processing x64-osx                        614 vs 630
     qt5-base                      **regression**                Fail vs Pass
 Processing x64-uwp                        435 vs 444
     libjpeg-turbo                 **regression**                Fail vs Pass

@Rastaban
Copy link
Contributor

Rastaban commented May 2, 2019

I think the only thing holding back this PR is the vcpkg CMAKE_SYSTEM_PROCESSOR bug.

@vicroms
Copy link
Member

vicroms commented May 2, 2019

@Rastaban

The fix in #5697 causes many regressions that I haven't had the time to look into, if there's a local workaround that could enable this PR to be merged it would be a preferable short-term solution.

@Rastaban Rastaban merged commit d53488c into microsoft:master May 6, 2019
@UnaNancyOwen
Copy link
Contributor Author

@Rastaban @vicroms Thanks!

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.

None yet

5 participants