Reuse png metadata handling of imsave() in FigureCanvasAgg.print_png(). #15435
Conversation
|
Now also includes the warning on metadata/pnginfo collision suggested in #15434 (comment). This effectively requires moving the generation of the Software tag to imsave (as we otherwise don't know whether the tag was autogenerated or provided by the user). |
There was a problem hiding this comment.
This is a rather big change just for removing the duplication of metadata and pil_kwargs merging. In particular, replacing a simple Image.save() with the more general mpl.image.imsave()feels a bit risky and obscures the actual quite simple call (I didn't easily manage to follow if imsave() with the given parameters is equivalent to the original code).
Extracting a _merge_metadata_pil_kwargs() function seems to be a simpler solution. - Or did I miss any other important stuff that imsave() is now doing?
|
The calls are equivalent, although I readily admit that it's not obvious to follow them (the relevant point is at matplotlib/lib/matplotlib/cm.py Line 221 in eb8327d matplotlib/lib/matplotlib/cm.py Line 236 in eb8327d As suggested above one of the reasons to move the pil_kwargs handling logic in imsave() is to let mplcairo also take advantage of it (well, if I need to call private APIs in mplcairo I can do it too :) but I'd rather not). |
|
Not urgent by any means, but does need to get into 3.3 to prevent a (subtle) regression against 3.2, for which the same issue was fixed in #15434, so tagging as release critical. |
|
Would this be fixed by merging 3.2.x into master? If not this needs a re-base. |
|
rebased |
| # semantics of duplicate keys in pnginfo is unclear. | ||
| if "pnginfo" in pil_kwargs: | ||
| if metadata: | ||
| cbook._warn_external("'metadata' is overridden by the " |
There was a problem hiding this comment.
I could also see a case for raising, but warning seems like a step in the right direction.
There was a problem hiding this comment.
I left this as is but can raise too, just pick one.
This avoids duplicating the conversion of metadata to PngInfo and revealed a bug in the priority between `metadata` and `pil_kwargs` in imsave(). Note that because `np.asarray(self.buffer_rgba())` is already a RGBA uint8 array, there is no colormapping step happening in imsave(). Ideally mplcairo should also be able to use imsave() for saving to png.
This avoids duplicating the conversion of metadata to PngInfo and
revealed a bug in the priority between
metadataandpil_kwargsinimsave(). (That bug is also fixed for 3.2 in #15434; this PR effectively
tests that.)
Note that because
np.asarray(self.buffer_rgba())is already a RGBAuint8 array, there is no colormapping step happening in imsave().
Ideally mplcairo should also be able to use imsave() for saving to png.
PR Summary
PR Checklist