Alex313031 / thorium

Chromium fork named after radioactive element No. 90. Windows and MacOS/Raspi/Android/Special builds are in different repositories, links are towards the top of the README.md.
https://thorium.rocks/
BSD 3-Clause "New" or "Revised" License
4.66k stars 143 forks source link

JPEG-XL support post M109 #104

Closed goodusername123 closed 4 days ago

goodusername123 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mo271 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

Alex313031 commented 1 year ago

@ziemek99 @tomByrer

Alex313031 commented 1 year ago

@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 commented 1 year ago

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

Alex313031 commented 1 year ago

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

Alex313031 commented 1 year ago

@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 commented 1 year ago

@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 > https://github.com/libjxl/libjxl/commit/6ae7cffd78eff8edc0df6f2a5ee23e43b31ab947 @mo271 @jonsneyers Are there any things that stand out or that you know of between https://github.com/libjxl/libjxl/commit/c27d499263435ac77007174e0f1cf54557cff23a and this revision as far as integration into chromium is concerned

gz83 commented 1 year ago

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 commented 6 months ago

@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 commented 6 months ago

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

gz83 commented 6 months ago

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