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

JPEG-XL support post M109 #104

Closed
goodusername123 opened this issue Feb 4, 2023 · 37 comments
Closed

JPEG-XL support post M109 #104

goodusername123 opened this issue Feb 4, 2023 · 37 comments

Comments

@goodusername123
Copy link

goodusername123 commented Feb 4, 2023

Is it planned to support JXL files after M109? it seems like a reversion patch could be made for this commit to restore support.

websites that that I currently know of that are shipping JXL files include the majority if not all sites using/made with Shopify (of which there is quite a bit) and also the gallery pages on a personal site made by a artist and photographer

also it seems like some of the chromium devs responsible for the original implementation are willing to provide support if stuff ends up breaking in future chromium milestones.

one of the reasons for me asking if continued support is planned is so I can know whether or not I can confidently recommend people over to Thorium as a alternative chromium fork that keeps JXL support post M109 and also to know if maybe it is eligible to be added to this list.

(edit) here are some websites that can be used to test JXL support listed below:

@gz83
Copy link
Collaborator

gz83 commented Feb 4, 2023

Is it planned to support JXL files after M109? it seems like a reversion patch could be made for this commit to restore support.

websites that that I currently know of that are shipping JXL files include the majority if not all sites using/made with Shopify (of which there is quite a bit) and also the gallery pages on a personal site made by a artist and photographer

also it seems like some official chromium devs are willing to provide support if stuff ends up breaking in future chromium milestones.

one of the reasons for me asking if continued support is planned is so I can know whether or not I can confidently recommend people over to Thorium as a alternative chromium fork that keeps JXL support post M109 and also to know if it is eligible to be added to this list.

@goodusername123 Thank you very much for your suggestion. There have been many discussions on crbug about whether the support for JPEG-XL should be removed. Different people hold different opinions. Personally, I still hope to keep this function.

According to the commit you provided, to restore the support for JPEG-XL requires a lot of code modification, and certain tests are required to ensure that the functions of the browser will not be broken.

Finally, the final decision on whether support for JPEG-XL should be restored after the M109 version still needs to be made by project leader @Alex313031 .

@goodusername123
Copy link
Author

I should mention that if a patch is created it's recommended to edit the "DEPS" file and change

'libjxl_revision': '8001738dc9cd8dc6fa24cf75fefd08f909b2ac3c',
to
'libjxl_revision': 'c27d499263435ac77007174e0f1cf54557cff23a',

and

'highway_revision': '8ae5b88670fb918f815b717c7c13d38a9b0eb4bb',
to
'highway_revision': '58746ca5b9f9444a2a3549704602ecc6239f8f41',

which should result in libjxl getting updated from 0.7.0rc to 0.8.1 and highway (which is required by libjxl) getting updated from 1.0.1rc to 1.0.3 hopefully.

@gz83
Copy link
Collaborator

gz83 commented Feb 5, 2023

I should mention that if a patch is created it's recommended to edit the "DEPS" file and change

'libjxl_revision': '8001738dc9cd8dc6fa24cf75fefd08f909b2ac3c', to 'libjxl_revision': 'c27d499263435ac77007174e0f1cf54557cff23a',

and

'highway_revision': '8ae5b88670fb918f815b717c7c13d38a9b0eb4bb', to 'highway_revision': '58746ca5b9f9444a2a3549704602ecc6239f8f41',

which should result in libjxl getting updated from 0.7.0rc to 0.8.1 and highway (which is required by libjxl) getting updated from 1.0.1rc to 1.0.3 hopefully.

@goodusername123
Regarding libjxl and highway, have the versions you mentioned been used by Chromium before?

I've currently added the libjxl code and files back to the latest Chromium branch, and I'm currently trying to combine it with Thorium's files to see if it compiles properly.

In addition, if the compilation is successful, and Alex accepts our actions and suggestions, we can bring libjxl back to Thorium after the M109 version

@gz83
Copy link
Collaborator

gz83 commented Feb 6, 2023

Is it planned to support JXL files after M109? it seems like a reversion patch could be made for this commit to restore support.

websites that that I currently know of that are shipping JXL files include the majority if not all sites using/made with Shopify (of which there is quite a bit) and also the gallery pages on a personal site made by a artist and photographer

also it seems like some official chromium devs are willing to provide support if stuff ends up breaking in future chromium milestones.

one of the reasons for me asking if continued support is planned is so I can know whether or not I can confidently recommend people over to Thorium as a alternative chromium fork that keeps JXL support post M109 and also to know if it is eligible to be added to this list.

@goodusername123 I have successfully brought JPEG-XL back to the new version of Thorium, and tested it on the website you provided, and the content that needs JPEG-XL can be loaded.

@jonsneyers
Copy link

It would be good to know if JPEG XL support will remain in Thorium after M109. I'm certainly hoping @Alex313031 can confirm this intention, since Thorium is currently the only browser with full JPEG XL support including HDR — HDR does not work properly in the four other browsers that currently have JPEG XL enabled by default, since they're all Firefox forks and Firefox doesn't support HDR in general. So for use cases like this one currently Thorium is the only option.

@gz83
Copy link
Collaborator

gz83 commented Feb 13, 2023

It would be good to know if JPEG XL support will remain in Thorium after M109. I'm certainly hoping @Alex313031 can confirm this intention, since Thorium is currently the only browser with full JPEG XL support including HDR — HDR does not work properly in the four other browsers that currently have JPEG XL enabled by default, since they're all Firefox forks and Firefox doesn't support HDR in general. So for use cases like this one currently Thorium is the only option.

@jonsneyers

When I added JPEG-XL back into the new version of Thorium, the whole process took me a long time (too many files to process). I think our entire Thorium development team has to consider whether doing so will increase our development pressure before making a decision about whether JPEG-XL should be added back into a new version of Thorium

@jonsneyers
Copy link

What can we do to reduce development pressure?

Would it help if we (libjxl devs) maintain an up-to-date chromium patch for jxl somewhere (e.g. on the chromium gerrit itself)?

@gz83
Copy link
Collaborator

gz83 commented Feb 13, 2023

What can we do to reduce development pressure?

Would it help if we (libjxl devs) maintain an up-to-date chromium patch for jxl somewhere (e.g. on the chromium gerrit itself)?

@jonsneyers

As you can see, Thorium has now been transferred from the tip-o-tree branch to the stable branch for development. After the M109 version, it may be transferred from the stable branch to the beta branch or dev branch for development.

At present, there are already a lot of files modified on Thorium. Every time we update, it takes several hours to rebase the repo (synchronize the upstream update).

If we add more files to restore JPEG-XL according to the patch above chromium gerrit (chromium deletes JPEG-XL from the code and deletes the corresponding simulation test from devtools), it may make the update time that used to take a long time has become longer, which is what I am worried about.

In addition, since we have not yet determined the development branch used by Thorium after the M109 version, if we decide to use some branches with less changes, the work mentioned above may become easier.

Making a patch to restore JPEG-XL is doable in my opinion, it would also help to restore JPEG-XL.

I have reported this issue and JPEG-XL to Alex, and I believe he will keep an eye on this issue and JPEG-XL.

Finally, I would like to thank everyone and the JPEG-XL development team for their support to Thorium.

@midzer
Copy link
Collaborator

midzer commented Feb 13, 2023

After watching talk https://www.youtube.com/watch?v=RYJf7kelYQQ from @jonsneyers I am certainly in favor of supporting JPEG XL long term in Thorium.

Until now, I've not heard of it's main features like "Responsive by design" and "Legacy friendly". Not checking compression ratios on my own and relying on the graph shown in the video, I think JPEG XL's approach is the way to go for the web to replace ancient JPEG.

@gz83
Copy link
Collaborator

gz83 commented Feb 13, 2023

This is the branch I maintained to restore JPEG-XL. At present, it is impossible to compile the new version of Thorium using this branch, and some files need to be updated

In addition, I have updated the highway in the upstream code tree, and the step of updating the highway should be skipped at present

https://github.com/gz83/Thorium-Rebase/tree/jxl

@gz83
Copy link
Collaborator

gz83 commented Feb 14, 2023

Today or tomorrow I will complete the rebase operation of the jxl branch

@mo271
Copy link

mo271 commented Feb 14, 2023

I will try to see if I can keep an up-to-date rebased version of libjxl in chrome on https://chromium-review.googlesource.com/ and let you know if this is done.

@gz83
Copy link
Collaborator

gz83 commented Feb 14, 2023

I will try to see if I can keep an up-to-date rebased version of libjxl in chrome on https://chromium-review.googlesource.com/ and let you know if this is done.

@mo271 Thanks a lot for your help!

@gz83
Copy link
Collaborator

gz83 commented Feb 14, 2023

The jxl branch has just undergone an update, checking out the following commits should be able to combine jxl and Thorium files in the repo for compilation.

commit ecc9df48e429ef865536a91c25f3f580ddcf0016 (HEAD)

Quota: Cleanup use of PrefixStorageInfo deprecation message

PrefixStorageInfo has been removed in M110. This change
cleans up the deprecation message for this feature.

@gz83
Copy link
Collaborator

gz83 commented Feb 14, 2023

@mo271

I just updated the repo again and tried to compile a version with JPEG-XL with the updated files, but after running the gn args command, I got the following error.

I'm looking for a solution, besides, if I'm not wrong, I found that my last few commits should be fine.

E:\chromium\src>gn args out\thorium
Waiting for editor on "E:\chromium\src\out\thorium\args.gn"...
Generating files...
ERROR at //third_party/libjxl/BUILD.gn:27:13: Undefined identifier
defines = libjxl_version_defines + [
^---------------------
See //third_party/blink/renderer/platform/BUILD.gn:1825:15: which caused the file to be included.
deps += [ "//third_party/libjxl:libjxl" ]
^----------------------------

@Alex313031
Copy link
Owner

@mo271 @jonsneyers This is amazing that you guys are even here, lol. After talking with @gz83 and seeing @midzer excitement, and the fact that as @jonsneyers pointed out, thorium is currently the only browser with full jpeg-xl support, I want to make it an official goal to keep libjxl post M109. If one of you could make a patch, whether on gerrit or elsewhere, that would be really awesome!

@gz83 What milestone are you building at, and did you rebase anything. That build.gn is a file thorium uses.

@gz83
Copy link
Collaborator

gz83 commented Feb 15, 2023

@mo271 @jonsneyers This is amazing that you guys are even here, lol. After talking with @gz83 and seeing @midzer excitement, and the fact that as @jonsneyers pointed out, thorium is currently the only browser with full jpeg-xl support, I want to make it an official goal to keep libjxl post M109. If one of you could make a patch, whether on gerrit or elsewhere, that would be really awesome!

@gz83 What milestone are you building at, and did you rebase anything. That build.gn is a file thorium uses.

@Alex313031

I am trying to compile in the following commit

commit ecc9df48e429ef865536a91c25f3f580ddcf0016 (HEAD)

I checked the two files involved in the error message yesterday, and found no problems. I will continue to check the relevant files today.

This is the branch I'm working on

https://github.com/gz83/Thorium-Rebase/tree/jxl

@mo271
Copy link

mo271 commented Feb 15, 2023

I was able to build the following commit locally:
https://chromium-review.googlesource.com/c/chromium/src/+/4255409/

This is a commit on top of current origin/main of chromium. It uses the last libjxl that was in chrome together with the libhighway that is currently in chromium (it had been updated in between..)

@gz83
Copy link
Collaborator

gz83 commented Feb 15, 2023

I was able to build the following commit locally: https://chromium-review.googlesource.com/c/chromium/src/+/4255409/

This is a commit on top of current origin/main of chromium. It uses the last libjxl that was in chrome together with the libhighway that is currently in chromium (it had been updated in between..)

@mo271 Thank you very much! ! ! I will try to compile a new version of Thorium with this patch and Thorium's files later on my local computer

@gz83
Copy link
Collaborator

gz83 commented Feb 16, 2023

I was able to build the following commit locally: https://chromium-review.googlesource.com/c/chromium/src/+/4255409/

This is a commit on top of current origin/main of chromium. It uses the last libjxl that was in chrome together with the libhighway that is currently in chromium (it had been updated in between..)

@mo271

I created two patches for restoring libjxl on gerrit. I used cherry-pick to apply them to the local code and combine them with Thorium's files, and found that the errors mentioned above still occurred.

E:\chromium\src>gn args out\thorium
Waiting for editor on "E:\chromium\src\out\thorium\args.gn"...
Generating files...
ERROR at //third_party/libjxl/BUILD.gn:27:13: Undefined identifier
defines = libjxl_version_defines + [
^---------------------
See //third_party/blink/renderer/platform/BUILD.gn:1825:15: which caused the file to be included.
deps += [ "//third_party/libjxl:libjxl" ]
^----------------------------

Below are the links to the two patches:
https://chromium-review.googlesource.com/c/chromium/src/+/4257656

https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4257582

Edit:
1.Applying these two patches in a pure chromium repo also gives the above error
2.Then, I switched to the patch you provided and found that the above error was still reported, and I am now compiling natively on a windows system.

@mo271
Copy link

mo271 commented Feb 16, 2023

I patched it a bit, since for one file the merge was botched. Should work better now. There are still some failing tests, probably due to the highway update, which I will fix when I find the time

@gz83
Copy link
Collaborator

gz83 commented Feb 16, 2023

I patched it a bit, since for one file the merge was botched. Should work better now. There are still some failing tests, probably due to the highway update, which I will fix when I find the time

@mo271

Thank you very much for your work. Now I can compile on the windows system, but the following error occurs during the compilation process. I need to manually remove the imported header file before the compilation can continue.

../../third_party/libjxl/src/lib/include\jxl/decode.h(26,10): fatal error: 'jxl/version.h' file not found
#include "jxl/version.h"
^~~~~~~~~~~~~~~
1 error generated.

Edit:
A patch for image type emulation in devtools also causes compilation errors and breaks, I need to spend a little more time investigating

In addition, for the trybots reporting errors in gerrit, can updating libjxl to the latest release version solve these errors?

@gz83
Copy link
Collaborator

gz83 commented Feb 16, 2023

@mo271 @midzer @jonsneyers @Alex313031 @goodusername123

The new version of Thorium with jxl has just been compiled. After testing, the browser itself and the jxl component work normally. The results of the jxl test are released below

Snipaste_2023-02-17_04-34-02

Snipaste_2023-02-17_04-37-09

saklistudio com_jxltests_1

The results of the tests should all look as expected

Browser Version:
image

Right now, I think there's still some work to be done restoring jxl, including what Moritz is doing to fix some failing tests and my proposed restoration of image type emulation in devtools.(Image type emulation issues were fixed)

@gz83
Copy link
Collaborator

gz83 commented Feb 22, 2023

@mo271

Moritz, I now compile libjxl files with Thorium's files. I encountered these problems in the process of compilation. Please check the following questions when you are free.

  1. The version.h header file introduced in src/lib/include/jxl/decode.h cannot be found during compilation, which causes compilation interruption

  2. The following errors occur in src/lib/include/jxl/decode.h, causing compilation interruption
    error: unknown type name 'jxlbitDepth'
    JXLDECODERSETIMAGEOUTBITDEPTH (JXLDECODER* DEC, Const JXLBITDEPTH* bit_depth);

I have deleted the header file and code mentioned above, and compiled no longer report errors.

@mo271
Copy link

mo271 commented Feb 22, 2023

@mo271

Moritz, I now compile libjxl files with Thorium's files. I encountered these problems in the process of compilation. Please check the following questions when you are free.

  1. The version.h header file introduced in src/lib/include/jxl/decode.h cannot be found during compilation, which causes compilation interruption
  2. The following errors occur in src/lib/include/jxl/decode.h, causing compilation interruption
    error: unknown type name 'jxlbitDepth'
    JXLDECODERSETIMAGEOUTBITDEPTH (JXLDECODER* DEC, Const JXLBITDEPTH* bit_depth);

I have deleted the header file and code mentioned above, and compiled no longer report errors.

I haven't look into it in details, but perhaps that decode.h file is generated dynamically depending on how you build it?!
The second error (and probably also the first?) sounds a bit like you are building with a version of libjxl that is to new. For now I would try building with the exact version that is referenced in my chromium patch. I will try to update libjxl to version 0.8.1 sometimes.

@gz83
Copy link
Collaborator

gz83 commented Feb 22, 2023

@mo271
Moritz, I now compile libjxl files with Thorium's files. I encountered these problems in the process of compilation. Please check the following questions when you are free.

  1. The version.h header file introduced in src/lib/include/jxl/decode.h cannot be found during compilation, which causes compilation interruption
  2. The following errors occur in src/lib/include/jxl/decode.h, causing compilation interruption
    error: unknown type name 'jxlbitDepth'
    JXLDECODERSETIMAGEOUTBITDEPTH (JXLDECODER* DEC, Const JXLBITDEPTH* bit_depth);

I have deleted the header file and code mentioned above, and compiled no longer report errors.

I haven't look into it in details, but perhaps that decode.h file is generated dynamically depending on how you build it?! The second error (and probably also the first?) sounds a bit like you are building with a version of libjxl that is to new. For now I would try building with the exact version that is referenced in my chromium patch. I will try to update libjxl to version 0.8.1 sometimes.

@mo271

The encode.h file also has a version.h file. There is no error reported in encode.h, but an error is reported in the decode.h file. I also found a cmake file in libjxl's file that might be related, it's unclear to me if it's related.

I am currently compiling with version 0.8.1 of libjxl, the first few times only the error related to version.h will appear, this error will not appear

error: unknown type name 'jxlbitDepth'
JXLDECODERSETIMAGEOUTBITDEPTH (JXLDECODER* DEC, Const JXLBITDEPTH* bit_depth);

@Alex313031
Copy link
Owner

https://github.com/Alex313031/thorium-libjxl

@Alex313031
Copy link
Owner

@ziemek99 @tomByrer

@Alex313031
Copy link
Owner

Alex313031 commented Mar 2, 2023

@mo271 @midzer @jonsneyers @gz83 @goodusername123 @earthsound Thorium M110 is out, with full jpeg-xl support! (And it is built with libjxl 8.0, instead of 7.2) > https://github.com/Alex313031/Thorium/releases/tag/M110.0.5481.178

@Daasin
Copy link

Daasin commented Mar 4, 2023

This would make Thorium my default browser instantly even on mobile if possible

@Alex313031
Copy link
Owner

@Daasin I plan on making a new android build soon.

@Alex313031
Copy link
Owner

@mo271 @midzer @jonsneyers @gz83 Thorium now has libjxl in M110 and M111, and will continue to have it for as long as we can keep the patches working. If noone else has anything else to comment, I will be closing this issue in a few days.

@Alex313031
Copy link
Owner

@mo271 @midzer @jonsneyers @goodusername123 @yochananmarqos Thorium M112 still has working jpeg xl support.

For thorium 113 I would like to update libhighway to the non rc version of 1.0.3
and jibjxl to > libjxl/libjxl@6ae7cff
@mo271 @jonsneyers Are there any things that stand out or that you know of between libjxl/libjxl@c27d499 and this revision as far as integration into chromium is concerned

@gz83
Copy link
Collaborator

gz83 commented May 3, 2023

In M112, I have updated highway to 1.0.3 (which is the version from M112 to M115 for the time being), in addition, the latest version released upstream now is 1.0.4, I think we can try to update highway to 1.0.4, and then I think libjxl can not be updated first, because upstream has not released a new version to the public yet.

The following patch was used at the time to update highway in chromium:
https://chromium-review.googlesource.com/c/chromium/src/+/4223354

@Alex313031

@Alex313031
Copy link
Owner

@mo271 @jonsneyers @gz83 @goodusername123 @Daasin libhighway was updated to 1.0.7, and libjxl was updated to 0.9.2 in the latest M121 release. It works well, however, something else in Thorium's code is causing segfaults, and I am working on it now before we build releases.
When it is done, maybe someone could test the HDR compliance, as some people said images were too bright.

@Firepal
Copy link

Firepal commented Feb 24, 2024

Is JPEG XL supposed to work on an Android build of Thorium? arm64 M121 build doesn't display JPEG XL images for me.

@gz83
Copy link
Collaborator

gz83 commented Feb 24, 2024

Is JPEG XL supposed to work on an Android build of Thorium? arm64 M121 build doesn't display JPEG XL images for me.

Already in the to-do list

@Firepal

@gz83 gz83 closed this as completed Sep 2, 2024
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

No branches or pull requests

8 participants