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

[api-minor] Remove the WebGL implementation #13358

Merged
merged 1 commit into from May 9, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented May 9, 2021

Reasons for the removal include:

  • This functionality was always somewhat experimental and has never been enabled by default, partly because of worries about rendering bugs caused by e.g. bad/outdated graphics drivers.

  • After the initial implementation, in PR WebGL and misc memory optimizations #4286 (back in 2014), no additional functionality has been added to the WebGL implementation.

  • The vast majority of all documents do not benefit from WebGL rendering, since only a couple of specific features are supported (e.g. some Soft Masks and Patterns).

  • There is, and has always been, zero test-coverage for the WebGL implementation.

  • Overall performance, in the PDF.js library, has improved since the experimental WebGL implementation was added.

Rather than shipping unused and untested code, it seems reasonable to simply remove the WebGL implementation for now; thanks to version control it's always possible to bring back the code should the need ever arise.

Fixes #5161


Edit: A new version was pushed to fix typos in the commit message, no actual code changes were made.

@Snuffleupagus Snuffleupagus marked this pull request as ready for review May 9, 2021 14:32
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented May 9, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/ef4a17b50f3c81d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 9, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/5a0834d707c0085/output.txt

Reasons for the removal include:
 - This functionality was always somewhat experimental and has never been enabled by default, partly because of worries about rendering bugs caused by e.g. bad/outdated graphics drivers.

 - After the initial implementation, in PR 4286 (back in 2014), no additional functionality has been added to the WebGL implementation.

 - The vast majority of all documents do not benefit from WebGL rendering, since only a couple of *specific* features are supported (e.g. some Soft Masks and Patterns).

 - There is, and has always been, *zero* test-coverage for the WebGL implementation.

 - Overall performance, in the PDF.js library, has improved since the experimental WebGL implementation was added.

Rather than shipping unused *and* untested code, it seems reasonable to simply remove the WebGL implementation for now; thanks to version control it's always possible to bring back the code should the need ever arise.
@pdfjsbot
Copy link

pdfjsbot commented May 9, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/ef4a17b50f3c81d/output.txt

Total script time: 25.94 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented May 9, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/5a0834d707c0085/output.txt

Total script time: 28.74 mins

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

Image differences available at: http://3.101.106.178:8877/5a0834d707c0085/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit f07d50f into mozilla:master May 9, 2021
@timvandermeij
Copy link
Contributor

Thanks!

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

Successfully merging this pull request may close these issues.

Enable WebGL by default?
3 participants