[MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" #878

hackermd · 2019-07-08T12:45:04Z

The Pixel Data element of DICOM image instances may contain JPEG bitstreams that were not converted to YCbCr color space upon compression. In these cases the value of PhotometricInterpretation is "RGB" rather than "YBR_FULL_422".

JPEG is color blind, i.e. the JPEG bitstream may not contain any information about any color transformation, and Pillow's JpegImagePlugin assumes that images have been transformed into YCbCr space before compression. As a consequence, the pillow_handler doesn't decode images correctly, which were compressed directly in RGB color space (see python-pillow/Pillow#3952).

This PR fixes the color "mode" of the Pillow.Image object by providing additional color information to the decoder and thereby ensure that images are decoded correctly.

…m into bugfix/pillow-color-handling

pep8speaks · 2019-07-08T12:45:06Z

Hello @hackermd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-17 02:11:38 UTC

hackermd · 2019-07-08T12:46:18Z

I noticed that the test_PI_RGB test now fails. Are we sure that this test case has been correct in the first place?

darcymason · 2019-07-08T21:14:23Z

I noticed that the test_PI_RGB test now fails. Are we sure that this test case has been correct in the first place

I'm not sure ... @rhaxton might know ... from quick check it may come from #521 but I didn't really follow the history fully.

rhaxton · 2019-07-11T22:50:42Z

The test_PI_RGB tests just test that the reading of lossy compressed jpegs behaves the same way that using dcmtk's dcmcjpeg and dcmdjpeg do.

The filenames being tested include the command line options passed to dcmcjpeg (version 3.6.1 as indicated in the file metadata). For example:

SC_rgb_dcmtk_+eb+cy+np.dcm

was created by running:

dcmcjpeg +eb +cy +np SC_rgb.dcm SC_rgb_dcmtk_+eb+cy+np.dcm

and the expected result was created by running:

dcmdjpeg SC_rgb_dcmtk_+eb+cy+np.dcm SC_rgb_dcmtk_ebcynp_dcmd.dcm

These tests only assert that we decode these files the same way dcmtk decodes them.

as noted in the discussion of #521, the gdcm and dcmtk command line tools disagreed in some of these cases. The dcmtk results "looked" better, so that is what is in the test (and what is supported).

If these tests are failing, then the pillow handler is not replicating dcmtk's dcmdjpeg command line tool for that file. Whether it is correct or not is an open question. I would like to note that quite a lot of lossy compressed color jpeg DICOM files have the PI tag set to "RGB" incorrectly, and we should have some way to support them....

hackermd · 2019-07-11T23:55:47Z

I would like to note that quite a lot of lossy compressed color jpeg DICOM files have the PI tag set to "RGB" incorrectly, and we should have some way to support them....

I think you are right about the incorrectly set "RGB". If the set the value of PhotometricInterpretation of the data set contained in file SC_rgb_dcmtk_+eb+cr.dcm to "YBR_FULL_422", then the pillow_handler.py implementation in this PR decodes the pixel data correctly.

I really don't like the idea of supporting invalid files, since this will lead to unexpected behavior down the line because users assume that files are valid and are then surprised if a different application fails to handle them.
If a file is invalid, the pixel handler should not try to provide a workaround IMO, but rather fail by raising an appropriate exception. I updated the test case and removed the invalid file (ab9c3fc).

hackermd · 2019-10-11T17:59:45Z

@darcymason @rhaxton Are there any objections against merging this PR?

scaramallion · 2019-10-12T02:48:22Z

There's no test coverage of the actual correction.

pydicom/pixel_data_handlers/pillow_handler.py

…m into bugfix/pillow-color-handling

hackermd · 2020-02-17T08:21:14Z

@scaramallion I added a test case via 9cb6fc2. It would great if this bug fix could make it into the next release.

…ix/pillow-color-handling

codecov · 2020-07-22T22:27:50Z

Codecov Report

Merging #878 (a0c3307) into master (0949488) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #878   +/-   ##
=======================================
  Coverage   95.22%   95.23%           
=======================================
  Files          60       60           
  Lines        9682     9691    +9     
=======================================
+ Hits         9220     9229    +9     
  Misses        462      462

Impacted Files	Coverage Δ
pydicom/dataset.py	`97.70% <ø> (ø)`
pydicom/pixel_data_handlers/pillow_handler.py	`95.04% <100.00%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0949488...a0c3307. Read the comment docs.

scaramallion · 2020-07-22T22:53:11Z

The two test datasets you added aren't based on the same image as the other SC* datasets, they should be renamed and their source and an explanation of what they contain/how they're encoded be added to the README in the test_files directory.

…m into bugfix/pillow-color-handling

hackermd · 2020-11-27T22:20:10Z

@scaramallion I renamed the file and added a description to the README as requested. Please let me know if anything else is required for getting this merged. I think it's important to get this fix released.

Image without the fix (pydicom 2.0.0):

Image with fix (b88d9b6):

* some changes to PR workflow, harmonize flags between the two * flake8 for only the changed files * start pymedphys pydicom-only test setup, but not active currently

Remove unicode literals no longer needed since dropping Python 2

* Install pymedphys[tests] * Add pymedphys cache

* add download.py, delete test files that are hosted elsewhere * update data manager to filter the zenodo files as if they were still within that directory * add installation notes

* fix-1062 * Revert explicit comparisons, add check that value is set * Add test

* Refactor codedict._filtered * use generators/lazy evaluation where possible

* Added Dataset.copy() method - used method inherited from dict before, which always returned empty dict - fixes #1146 - adapt test for shallow/deep copy

* Move larger test files to separate repo (pydicom/pydicom-data) * update github actions scripts to cache the .pydicom data location * Change to commit hash for urls

* Add JLS tests * Add as_array() test * Add versioning info

…m into bugfix/pillow-color-handling

…or-handling

hackermd · 2021-01-10T16:12:14Z

@scaramallion I updated the branch. First tried to rebase, but gave up after rebasing for over half an hour. Ultimately, ended up merging current master into the branch.

Tests are passing. It would really be great if you could get this merged.

scaramallion · 2021-04-12T00:08:28Z

Dang, I thought that would re-run the CI

…ix/pillow-color-handling

hackermd added 3 commits July 8, 2019 07:46

Handle JPEG-compressed RGB color images correctly

52724b1

Handle JPEG-compressed RGB color images correctly

5e8c867

Merge branch 'bugfix/pillow-color-handling' of github:hackermd/pydico…

97e222f

…m into bugfix/pillow-color-handling

Remove test case of invalid rgb jpeg file

ab9c3fc

Update jpeg rgb test case for gdcm handler

36e1306

scaramallion reviewed Oct 12, 2019

View reviewed changes

pydicom/pixel_data_handlers/pillow_handler.py Show resolved Hide resolved

hackermd and others added 8 commits October 12, 2019 12:07

Add docstring to pixel handler function

7f08120

Handle JPEG-compressed RGB color images correctly

4e749ee

Remove test case of invalid rgb jpeg file

a82d700

Update jpeg rgb test case for gdcm handler

df991b1

Update pillow pixel data handler for version 1.4

9d6cbf3

Merge branch 'bugfix/pillow-color-handling' of github:hackermd/pydico…

ac061d9

…m into bugfix/pillow-color-handling

Add test for rgb jpeg encoded color images

9cb6fc2

Remove comment

eca1f89

Merge branch 'master' of https://github.com/pydicom/pydicom into bugf…

7bc5091

…ix/pillow-color-handling

scaramallion approved these changes Jul 23, 2020

View reviewed changes

hackermd added 2 commits November 27, 2020 17:15

Rename and describe test file for RGB JPEG

b88d9b6

Merge branch 'bugfix/pillow-color-handling' of github:hackermd/pydico…

41d6330

…m into bugfix/pillow-color-handling

Handle JPEG-compressed RGB color images correctly

69e82d8

darcymason and others added 19 commits January 9, 2021 18:43

Add workflow for push to master, expanded CI matrix (#1123)

60fdc78

* some changes to PR workflow, harmonize flags between the two * flake8 for only the changed files * start pymedphys pydicom-only test setup, but not active currently

Drop unicode literals (#1124)

4995628

Remove unicode literals no longer needed since dropping Python 2

Update zenodo DOI for v2.0 release (#1126)

723268c

Add pymedphys tests to Master workflow (#1129)

81646af

* Install pymedphys[tests] * Add pymedphys cache

Add type hints for uid module (#1131)

3e09b86

Use chain.from_iterable in codedict.py (#1132)

3dd5485

House large test data on Zenodo instead of within the package (#1114)

0e75ce4

* add download.py, delete test files that are hosted elsewhere * update data manager to filter the zenodo files as if they were still within that directory * add installation notes

Check is_little_endian and is_implicit_VR in write_dataset() (#1135)

cf8a2f3

* fix-1062 * Revert explicit comparisons, add check that value is set * Add test

Refactor codedict._filtered (#1134)

70ef87b

* Refactor codedict._filtered * use generators/lazy evaluation where possible

Added Dataset.copy() method (#1151)

926d30e

* Added Dataset.copy() method - used method inherited from dict before, which always returned empty dict - fixes #1146 - adapt test for shallow/deep copy

Move larger test files to separate repo (#1145)

14e981c

* Move larger test files to separate repo (pydicom/pydicom-data) * update github actions scripts to cache the .pydicom data location * Change to commit hash for urls

Add pylibjpeg pixel data handler (#1128)

c737554

* Add JLS tests * Add as_array() test * Add versioning info

Add UID constants for MPEG transfer syntaxes (#1155)

883d606

Fix GDCM handler not working with BytesIO (#1157)

f40b7ca

Update example files

4732fe7

Fix flake8 issue

bfc6ff2

Ensure that pixel handler is not None

41cf07e

Merge branch 'bugfix/pillow-color-handling' of github:hackermd/pydico…

4abc71f

…m into bugfix/pillow-color-handling

Merge remote-tracking branch 'upstream/master' into bugfix/pillow-col…

4179a87

…or-handling

scaramallion closed this Apr 12, 2021

scaramallion reopened this Apr 12, 2021

scaramallion added 3 commits April 17, 2021 12:08

Merge branch 'master' of https://github.com/pydicom/pydicom into bugf…

6dcf553

…ix/pillow-color-handling

Add to release notes

b0af7f0

Fix typing

9af6919

scaramallion changed the title ~~Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB"~~ [MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" Apr 17, 2021

scaramallion merged commit 7fb7773 into pydicom:master Apr 17, 2021

Readd file

a0c3307

hackermd mentioned this pull request Jul 17, 2021

Fix decoding of RGB JPEG with Adobe APP14 marker #1444

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" #878

[MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" #878

hackermd commented Jul 8, 2019

pep8speaks commented Jul 8, 2019 •

edited

Loading

hackermd commented Jul 8, 2019

darcymason commented Jul 8, 2019

rhaxton commented Jul 11, 2019 •

edited

Loading

hackermd commented Jul 11, 2019

hackermd commented Oct 11, 2019

scaramallion commented Oct 12, 2019 •

edited

Loading

hackermd commented Feb 17, 2020

codecov bot commented Jul 22, 2020 •

edited

Loading

scaramallion commented Jul 22, 2020 •

edited

Loading

hackermd commented Nov 27, 2020

hackermd commented Jan 10, 2021

scaramallion commented Apr 12, 2021

[MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" #878

[MRG] Fix decoding of JPEG compressed color images with Photometric Interpretation "RGB" #878

Conversation

hackermd commented Jul 8, 2019

pep8speaks commented Jul 8, 2019 • edited Loading

Comment last updated at 2021-04-17 02:11:38 UTC

hackermd commented Jul 8, 2019

darcymason commented Jul 8, 2019

rhaxton commented Jul 11, 2019 • edited Loading

hackermd commented Jul 11, 2019

hackermd commented Oct 11, 2019

scaramallion commented Oct 12, 2019 • edited Loading

hackermd commented Feb 17, 2020

codecov bot commented Jul 22, 2020 • edited Loading

Codecov Report

scaramallion commented Jul 22, 2020 • edited Loading

hackermd commented Nov 27, 2020

hackermd commented Jan 10, 2021

scaramallion commented Apr 12, 2021

pep8speaks commented Jul 8, 2019 •

edited

Loading

rhaxton commented Jul 11, 2019 •

edited

Loading

scaramallion commented Oct 12, 2019 •

edited

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

scaramallion commented Jul 22, 2020 •

edited

Loading