Fix dpi issue for bitmaps on the OS X backend #1230
Conversation
Bitmaps were being generated from the NSView. NSImages, by default, always use a dpi of 72. To generate a bitmap of a different size, the image must be redrawn into a new rect. To resample the image at a different resolution, a conversion is made to use the new dpi.
|
Since this is quite an invasive fix, I'd like Michiel's feedback on this before it gets merged. |
|
@mdehoon, in case you were not aware of this PR, please take a look when you can. |
|
I don't see anything obviously wrong with this pull request, so assuming it passed sufficient testing I would suggest to accept it. |
|
@mdehoon Thanks for taking the time to look at the code! I really appreciate your thoughts. It would be nice to fix this long-standing issue with the OS X backend. Perhaps this could make it into 1.2.x? Or, for want of not not being too hasty, milestoning it for 1.2.x and letting it garner more input I think would also be beneficial. |
|
I agree that it would be nice to get it into 1.2 and be done with it. I don't have time for any testing now. I assume @dmcdougall has tested for basic functionality with all file types. If so, then the only other important test I can think of would be to run it on each file type in a long loop to check for memory leakage. Ideally this would be done on a range of OSX versions and architectures, but I think that waiting for "someone" to do this would be counterproductive. I would be satisfied with a single test for memory leakage with 1000 or so iterations on a reasonably up-to-date system. @dmcdougall, can you give that a try? We have cbook.report_memory for exactly this purpose. |
|
@efiring Sure thing. I actually had only tested TIFF and BMP. I will check all files in a loop, as you suggest, and I'll report back here with stats when I'm done. Watch this space. |
|
Ok, here goes. Here's the script I wrote to check each bitmap type: #!/opt/local/bin/python
import matplotlib
matplotlib.use('macosx')
import matplotlib.cbook as cbook
import matplotlib.pyplot as plt
cbook.report_memory()
fig = plt.figure()
ax = fig.add_subplot(1, 1, 1)
ax.plot([1, 2, 3])
filetypes = ['tiff', 'bmp', 'jpg', 'gif', 'png']
i = 0
for f in filetypes:
while i < 1000:
if i == 1:
print 'Begin: Type', f, 'Mem usage', cbook.report_memory()
if i == 999:
print 'End: Type', f, 'Mem usage', cbook.report_memory()
fig.savefig('fig.'+f)
i += 1
i = 0Here's the stats before the change: $ ./bitmap_memtest.py # before
Begin: Type tiff Mem usage 37808
End: Type tiff Mem usage 39156
Begin: Type bmp Mem usage 38212
End: Type bmp Mem usage 39332
Begin: Type jpg Mem usage 39336
End: Type jpg Mem usage 39336
Begin: Type gif Mem usage 40408
End: Type gif Mem usage 40432
Begin: Type png Mem usage 42972
End: Type png Mem usage 45112Here's the change after: $ ./bitmap_memtest.py # after
Begin: Type tiff Mem usage 38908
End: Type tiff Mem usage 39376
Begin: Type bmp Mem usage 39376
End: Type bmp Mem usage 39376
Begin: Type jpg Mem usage 37496
End: Type jpg Mem usage 37512
Begin: Type gif Mem usage 41388
End: Type gif Mem usage 41856
Begin: Type png Mem usage 42332
End: Type png Mem usage 44580Though it increases steadily, it increases the same in each case. I conclude that I introduced no (new) memory leaks. |
|
I also did a quick |
|
Let's try to fix the memory leak. This is a good test case, and without fixing it we don't know if we added another memory leak by this bug fix. |
I agree with the sentiment, but I would encourage whoever looks into this to do it as a separate package of work. (in most cases: many smaller PRs are better than one big one) |
|
Yes, the memory leak needs to be fixed. I was deliberating whether to fix it in this pull request or not. I have some ideas of where to look. Since I already have the script, I'd be happy to look into this further. |
|
Just a quick reminder, a couple of days ago, a user on the mailing list was |
|
Ok, well the results above were for numpy version 1.6.2. Here's some more: $ ./bitmap_memtest.py # numpy 1.6.1
Begin: Type tiff Mem usage 39056
End: Type tiff Mem usage 39416
Begin: Type bmp Mem usage 39416
End: Type bmp Mem usage 39552
Begin: Type jpg Mem usage 37676
End: Type jpg Mem usage 37672
Begin: Type gif Mem usage 41408
End: Type gif Mem usage 41880
Begin: Type png Mem usage 42452
End: Type png Mem usage 44616$ ./bitmap_memtest.py # numpy current master
Begin: Type tiff Mem usage 39372
End: Type tiff Mem usage 39648
Begin: Type bmp Mem usage 39648
End: Type bmp Mem usage 39652
Begin: Type jpg Mem usage 37772
End: Type jpg Mem usage 37772
Begin: Type gif Mem usage 41648
End: Type gif Mem usage 42188
Begin: Type png Mem usage 42692
End: Type png Mem usage 44864 |
$ ./bitmap_memtest.py # numpy 1.5.1
Begin: Type tiff Mem usage 38048
End: Type tiff Mem usage 38624
Begin: Type bmp Mem usage 38624
End: Type bmp Mem usage 38620
Begin: Type jpg Mem usage 36744
End: Type jpg Mem usage 36744
Begin: Type gif Mem usage 40480
End: Type gif Mem usage 40948
Begin: Type png Mem usage 41488
End: Type png Mem usage 43560 |
|
So, I found some memory leaks, but not in the OS X backend: They appear to be coming from the PyObject *module_ptr = new ExtensionModuleBasePtr( this );I have no idea where this pointer is freed. Does anybody more experienced have any idea? |
|
To keep things separate, I strongly suggest that memory leak identification gets its own issue. @dmcdougall : Is this code in this PR complete? If so, I will merge. Alternatively, if it depends on another issue, such as the memory leak one, then we can hold this PR until that one is complete. Cheers, |
|
I even see similar leaks in the Agg backend, though admittedly, not as bad. I think the source of the memory leak is deeper. I think this code is complete and this PR does not depend on another issue. The memory leak doesn't seem to be constrained to the mac backend. |
Fix dpi issue for bitmaps on the OS X backend
|
Merged to master. @efiring mentioned he may like to see this on 1.2.x, which would obviously need another piece of work (to open the PR). It may be worth clarifying here first if that is still the case. |
|
We don't need another PR to put it on 1.2.x -- it can just be merged there. I think I agree with @efiring that it should be. I'll go ahead and do that. |
Bitmaps were being generated from the NSView. NSImages, by default,
always use a dpi of 72. To generate a bitmap of a different size,
the image must be redrawn into a new rect. To resample the image at
a different resolution, a conversion is made to use the new dpi.
This is a first attempt at resolving issue #113.
I make no claim this is the only, or correct, way to do this. There may be other ways, but this code produces bone-fide legit 1000 dpi images:
It is now 1:45am and I have been bashing my head against the wall for about 4 hours trying to fix this. It is time for bed.
Feedback welcome :D