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.

Sign up for GitHub

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

Jump to bottom

Decompress when it's possible images in using DecompressionStream #18167

Merged
merged 1 commit into from
Jun 2, 2024
Merged

Decompress when it's possible images in using DecompressionStream #18167

merged 1 commit into from
Jun 2, 2024

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented May 24, 2024 •
edited
Loading

Getting images is already asynchronous, so we can use this opportunity
to use DecompressStream (which is async too) to decompress images.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/535be119a4f4313/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/867d30477989dc6/output.txt

github-advanced-security[bot]
github-advanced-security bot found potential problems May 24, 2024
View reviewed changes
src/core/jpx_stream.js Fixed Show fixed Hide fixed
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/867d30477989dc6/output.txt

Total script time: 27.70 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 177
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/867d30477989dc6/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/535be119a4f4313/output.txt

Total script time: 40.08 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 87

Image differences available at: http://54.193.163.58:8877/535be119a4f4313/reftest-analyzer.html#web=eq.log

@calixteman calixteman changed the title Decompress FlatDecode images in using DecompressStream stuff Decompress when it's possible images in using DecompressStream May 25, 2024
@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/44dda3c5db56aa0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/37708c476124253/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/44dda3c5db56aa0/output.txt

Total script time: 3.05 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.193.163.58:8877/44dda3c5db56aa0/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-windows test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/044c8423063e775/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/044c8423063e775/output.txt

Total script time: 11.68 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.193.163.58:8877/044c8423063e775/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/37708c476124253/output.txt

Total script time: 27.77 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 1
  different ref/snapshot: 15
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/37708c476124253/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-windows test

@mozilla mozilla deleted a comment from moz-tools-bot May 26, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 26, 2024
@calixteman
Copy link
Contributor Author

/botio-windows test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/25dde028f645c9b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/25dde028f645c9b/output.txt

Total script time: 44.43 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 7

Image differences available at: http://54.193.163.58:8877/25dde028f645c9b/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/6f2a2d4e5298eb0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/edde2664f074b69/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/6f2a2d4e5298eb0/output.txt

Total script time: 27.67 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 23

Image differences available at: http://54.241.84.105:8877/6f2a2d4e5298eb0/reftest-analyzer.html#web=eq.log

@calixteman calixteman marked this pull request as ready for review May 26, 2024 16:24
@calixteman calixteman requested a review from Snuffleupagus May 26, 2024 16:24
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/edde2664f074b69/output.txt

Total script time: 42.68 mins

  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 9

Image differences available at: http://54.193.163.58:8877/edde2664f074b69/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented May 28, 2024 •
edited
Loading

This seems like something that we shouldn't land just prior to a release, but leaving a couple of general questions below.

How does this affect performance of image decoding? Are there any relevant benchmarking results?

Also, in an earlier version of this patch there was what appears to be an intermittent error; see #18167 (comment) where the logs contain:

TEST-UNEXPECTED-FAIL | test failed pr12564 | in firefox | page1 round 1 | render : Error: Requesting object that isn't resolved yet img_p0_15.

Was this simply caused by a not fully working patch, or is there potentially some pre-existing code-path where we're not ensuring that all /Image resources are available before decoding starts?

@calixteman
Copy link
Contributor Author

Now we've all the pdf files in Talos, I'll run a try perf later and I'll provide the results.
That said locally with an image I added in using the add stamp feature and in using the profiler shew a x5 improvement when decompressing the image stream.

@calixteman calixteman force-pushed the decompress_image branch 2 times, most recently from dbba70c to eee68cc Compare June 1, 2024 15:56
@calixteman calixteman changed the title Decompress when it's possible images in using DecompressStream Decompress when it's possible images in using DecompressionStream Jun 1, 2024
@calixteman
Copy link
Contributor Author

I made a comparison:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=608a261305cc7a9eb91d35f503cdfc52fba5bf0c&originalSignature=21404&newSignature=21404&framework=1&application=firefox&originalRevision=e1200c902ee374fa26b90f918099a87fbc1e20d2&page=1

and there is almost no significant difference.
That said locally, with the profiler, the decompression is ~5x faster.
For example with bug1077808.pdf:

  • with patch: profile
  • without: profile
    If you look at function buildPaintImageXObject in the worker thread, the difference is noticeable.

Snuffleupagus
Snuffleupagus reviewed Jun 1, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts about the intermittent failure mentioned in #18167 (comment), since that one would be nice to understand?

src/core/base_stream.js Show resolved Hide resolved
src/core/flate_stream.js Outdated Show resolved Hide resolved
src/core/flate_stream.js Outdated Show resolved Hide resolved
src/core/flate_stream.js Outdated Show resolved Hide resolved
src/core/jbig2_stream.js Outdated Show resolved Hide resolved
src/core/jbig2_stream.js Outdated Show resolved Hide resolved
src/core/jpeg_stream.js Outdated Show resolved Hide resolved
src/core/jpeg_stream.js Outdated Show resolved Hide resolved
src/core/jpx_stream.js Outdated Show resolved Hide resolved
src/core/jpx_stream.js Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

Any thoughts about the intermittent failure mentioned in #18167 (comment), since that one would be nice to understand?

Yep my code was buggy :) so it shouldn't happen any more.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/bad646d74cd6e3c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/eb7b60d6d2db9ad/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/bad646d74cd6e3c/output.txt

Total script time: 27.98 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 17
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/bad646d74cd6e3c/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/eb7b60d6d2db9ad/output.txt

Total script time: 28.40 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 7

Image differences available at: http://54.193.163.58:8877/eb7b60d6d2db9ad/reftest-analyzer.html#web=eq.log

Snuffleupagus
Snuffleupagus approved these changes Jun 2, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, thank you!

src/core/flate_stream.js Outdated Show resolved Hide resolved
@calixteman
Decompress when it's possible images in using DecompressionStream
9654ad5 
Getting images is already asynchronous, so we can use this opportunity
to use DecompressStream (which is async too) to decompress images.
@calixteman calixteman merged commit 5c51d56 into mozilla:master Jun 2, 2024
9 checks passed
@calixteman calixteman deleted the decompress_image branch June 2, 2024 14:08
@Snuffleupagus Snuffleupagus mentioned this pull request Jun 16, 2024
Merged
@timvandermeij timvandermeij mentioned this pull request Jun 16, 2024
Closed
@ZeroXClem ZeroXClem mentioned this pull request Aug 12, 2024
Closed
@npanchal108 npanchal108 mentioned this pull request Aug 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
core image-conversion performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@calixteman @moz-tools-bot @Snuffleupagus @timvandermeij