Skip to content

Fix a "mode == 'RGBA'" - #626

Closed
hugovk wants to merge 2 commits into
python-pillow:masterfrom
hugovk:j2k
Closed

hugovk wants to merge 2 commits into
python-pillow:masterfrom
hugovk:j2k

Conversation

@hugovk

@hugovk hugovk commented Apr 16, 2014

Copy link
Copy Markdown
Member

PR #616 to clean code includes two functional changes (== that should be =) but without tests (requested by @wiredfool).

     elif csiz == 4:
        mode == 'RGBA'

Here's a test for the first one (line 48) that fails before any changes, and passes with == changed to =:

    mode == 'RGBA'
UnboundLocalError: local variable 'mode' referenced before assignment

Failing build: https://travis-ci.org/hugovk/Pillow/jobs/22942098
Passing build: https://travis-ci.org/hugovk/Pillow/jobs/22942992

I added two test images, created with ImageMagick on Windows. This fix is with the .j2k file. I was hoping the .jp2 would cover the other one but it didn't. *


I think the test images should be renamed somehow. Suggestions?

Please can someone else create a test .jp2 file for the other case? It needs to hit line 125.


* When I did some testing (on Windows), the .jp2's signature is different.

  • Actual: \x00\x00\x00\x0cjP \n\x87\n\x00
  • Expected: \x00\x00\x00\x0cjP \x0d\x0a\x87\x0a (line 145)

@hugovk hugovk mentioned this pull request Apr 16, 2014
@hugovk hugovk changed the title Fix a "mode == 'RGBA' Fix a "mode == 'RGBA'" Apr 16, 2014
@wiredfool

Copy link
Copy Markdown
Member

Thanks @hugovk.

Somewhere in the openjpeg documentation, I think I saw a set of sample images. Not sure that I'd want to use them all, as they were many times the size of our project. We might be able to use one/several of those to improve our test coverage.

@hugovk

hugovk commented Apr 16, 2014

Copy link
Copy Markdown
Member Author

Thanks, I found some at http://www.openjpeg.org/index.php?menu=samples

In another branch, I added the smallish Bretagne1.j2k (90 KB) and Cevennes2.jp2 (18 KB) but both are RGB and not RGBA so they didn't hit line 125. They increased the coverage by a single line. But I don't know the copyright status -- are they ok?

The others on that page are probably too big, so I didn't try them: Bretagne2.j2k (5 MB), Cevennes1.j2k (1.5 MB) and Rome.jp2 (379 KB).

@wiredfool wiredfool mentioned this pull request May 10, 2014
@wiredfool wiredfool closed this May 10, 2014
@hugovk
hugovk deleted the j2k branch May 10, 2014 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants