Do not transform jpeg RGB components #11967
Conversation
There was a problem hiding this comment.
Please also update the commit message to include all of the relevant information found in #11967 (comment), such that that information is available on e.g. the Git command-line as well.
Please remember to squash the commits once you've made changes, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits
test/pdfs/.gitignore
Outdated
| @@ -147,6 +147,7 @@ | |||
| !issue1055r.pdf | |||
| !issue11713.pdf | |||
| !issue1293r.pdf | |||
| !jpeg_rgb.pdf | |||
There was a problem hiding this comment.
Please change the name of the file to issue11931.pdf instead, which is the usual format used in the PDF.js code-base since it makes finding/associating the test-case with the original issue easier.
test/test_manifest.json
Outdated
| { "id": "jpeg_rgb", | ||
| "file": "pdfs/jpeg_rgb.pdf", |
There was a problem hiding this comment.
As outline above, please change to
{ "id": "issue11931",
"file": "pdfs/issue11931.pdf",
src/core/jpg.js
Outdated
| this.components[0].index === 0x52 && // ASCII R. G and B | ||
| this.components[1].index === 0x47 && | ||
| this.components[2].index === 0x42 |
There was a problem hiding this comment.
Nit: It's probably not a bad idea to change the comment to something like the following example instead (which is a somewhat common format in these types of situations throughout the code-base):
this.components[0].index === /* "R" = */ 0x52 &&
this.components[1].index === /* "G" = */ 0x47 &&
this.components[2].index === /* "B" = */ 0x42That way you'd ensure that the comment doesn't move and thus become more difficult to connect to the relevant code, which can sometimes be a problem if/when automatic reformatting happens (as triggered by updating the Prettier dependency).
src/core/jpg.js
Outdated
| @@ -991,8 +991,10 @@ var JpegImage = (function JpegImageClosure() { | |||
| var components = [], | |||
| component; | |||
| for (i = 0; i < selectorsCount; i++) { | |||
| var componentIndex = frame.componentIds[data[offset++]]; | |||
| var index = data[offset++]; | |||
There was a problem hiding this comment.
Nit: Please use let or const, as appropriate, when adding new code.
|
Updated commit message, test name, comments and the var to a const. Thanks |
…s "RGB" in ASCII to say it so On ISO/IEC 10918-6:2013 (E), section 6.1: (http://www.itu.int/rec/T-REC-T.872-201206-I/en) "Images encoded with three components are assumed to be RGB data encoded as YCbCr unless the image contains an APP14 marker segment as specified in 6.5.3, in which case the colour encoding is considered either RGB or YCbCr according to the application data of the APP14 marker segment" But common jpeg libraries consider RGB too if components index are ASCII R (0x52), G (0x47) and B (0x42): https://stackoverflow.com/questions/50798014/determining-color-space-for-jpeg/50861048 Issue mozilla#11931
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/f7a373e0edb796c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/aedd1150192f05e/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/f7a373e0edb796c/output.txt Total script time: 25.79 mins
Image differences available at: http://54.67.70.0:8877/f7a373e0edb796c/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/aedd1150192f05e/output.txt Total script time: 27.75 mins
Image differences available at: http://54.215.176.217:8877/aedd1150192f05e/reftest-analyzer.html#web=eq.log |
|
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/9d830ab145da83f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/4808cf3841e3203/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/9d830ab145da83f/output.txt Total script time: 23.98 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/4808cf3841e3203/output.txt Total script time: 25.41 mins
|
|
/botio-windows makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/c51062b9c20ec75/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/c51062b9c20ec75/output.txt Total script time: 25.93 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/04fd3d47e0d5c99/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/04fd3d47e0d5c99/output.txt Total script time: 3.31 mins Published |
|
Thank you for your contribution! |
Hello,
Fix issue #11931
The image is encoded using RGB, not YCbCr
here : https://stackoverflow.com/questions/50798014/determining-color-space-for-jpeg/50861048
On ISO/IEC 10918-6:2013 (E), section 6.1: (http://www.itu.int/rec/T-REC-T.872-201206-I/en)
Images encoded with three components are assumed to be RGB data encoded as YCbCr unless the image contains an APP14 marker segment as specified in 6.5.3, in which case the colour encoding is considered either RGB or YCbCr according to the application data of the APP14 marker segment
The head.jpg doesn't have a app14, so image seems to be using YCbCr not RGB
But, the second best answer points to look up the implementation of a Java jpeg library, seems that if components are called R, G and B, the space color is RGB, and this is the case, the components are called in this way.
This pull request maintain the index in the components struct to allow _isColorConversionNeeded to do not convert if the components are called R, G and B.
The Pdf is now showed correctly