Should `ImageItem` take its opacity into consideration when saving?

[ntjess]

import pyqtgraph as pg
import numpy as np
app = pg.mkQApp()
ii = pg.ImageItem()
ii.setOpacity(0)
ii.setImage(np.random.random((500,500,3)))
ii.save('./test.png')

I would argue this should be functionally equivalent to

ii.setImage(np.random.random((500,500,4)))
ii.image[...,3] = 0

Since it is visually equivalent.

But in the first example, the saved image is fully visible. Documentation indicates the visible image is saved, so I would think image opacity should be taken into account the same way. Currently I have to resort to a LUT with alpha proportional to image opacity before saving when it would be much easier to interface with setOpacity. Perhaps a separate argument could be provided to save?

[j9ac9k]

I don't use ImageItem, and use ImageView in minimal ways, but I think this is a good idea. I would totally accept a PR provided it didn't break existing functionality.

[pijyoi]

how about using pillow?

import numpy as np
import pyqtgraph as pg
from PIL import Image

data = np.random.random((500, 500, 3))
ii = pg.ImageItem(data)
ii.render()
pimg = Image.fromqimage(ii.qimage)
pimg.putalpha(0)
pimg.save('test.png')

[ntjess]

I'm not sure how this would affect images with alpha channels and image opacity (does it stack or overwrite?)

Something within pyqtgraph that worked for me was

def newSave(ii: pg.ImageItem, fileName: str='./test.png', *args, includeBorder=False, 
            useOpacity=False):
  im = pg.fn.makeQImage(np.zeros_like(ii.image))
  painter = QtGui.QPainter(im)
  oldBorder = ii.border
  if not includeBorder:
    ii.setBorder(None)
  if useOpacity:
    painter.setOpacity(ii.opacity())
  ii.paint(painter)
  ii.setBorder(oldBorder)
  painter.end()
  im.save(fileName, *args)

This preserves any alpha from LUT as well as RGBA channels in the underlying image data, and only adds additional image opacity when painting the qimage.

@j9ac9k if you like, I can submit a PR that changes save to perform this action instead of directly calling render(). This has the added benefits of also adding the image border, which the old save method did not. The default flag values would preserve past behavior if that is of interest.

[j9ac9k]

@j9ac9k if you like, I can submit a PR that changes save to perform this action instead of directly calling render(). This has the added benefits of also adding the image border, which the old save method did not. The default flag values would preserve past behavior if that is of interest.

I'm good with this, but I'd likely follow recommendations/input from some of the other maintainers/contributors that have worked with the image capability of the library more; as I really use the bare minimum of it, and I know some of the Acq4 folks may have strong opinions.

@outofculture @dgoeries @pijyoi you all have input?

[pijyoi]

@ntjess , have you tried your code on master? Quite a bit of changes due to #1466 and #1501

Also, ii.image can be any dtype and shape can be (h, w), (h, w, 1), (h, w, 3), (h, w, 4)

[ntjess]

@pijyoi revisions based on your comment:

import pyqtgraph as pg
import numpy as np
from pyqtgraph.Qt import QtGui, QtCore

class NewImageItem(pg.ImageItem):
  def save(self, fileName, *args, includeBorder=False, useOpacity=False):
    if self.image is None:
      # Warn? Raise error? Currently errs with AttributeError
      return
    pm = self.getPixmap()

    im = pm.toImage()
    im.fill(QtCore.Qt.transparent)
    painter = QtGui.QPainter(im)
    oldBorder = self.border
    if not includeBorder:
      self.setBorder(None)
    if useOpacity:
      painter.setOpacity(self.opacity())
    self.paint(painter)
    self.setBorder(oldBorder)
    painter.end()
    im.save(fileName, *args)

app = pg.mkQApp()
ii = NewImageItem()
ii.setBorder(pg.mkPen('r', width=5))
ii.setOpacity(0.7)
ii.setImage(np.random.random((500,500,3)))
ii.save('3chan.png', useOpacity=True, includeBorder=True)
ii.setImage(np.random.random((500,500,1)))
ii.save('1chan.png', useOpacity=True, includeBorder=True)
ii.setImage(np.random.random((500,500, 4)))
ii.save('4chan.png', useOpacity=True, includeBorder=True)
ii.setImage(np.random.random((500,500)))
ii.save('gray.png', useOpacity=True, includeBorder=True)

Since the function is being modified anyway, a few questions:

• Is the current behavior when image is None desired, e.g. throwing an AttributeError on NoneType?
• Would it be worthwhile to add toBytes and copy to the call signature as they exist in the ImageExporter?

[pijyoi]

getPixmap() calls render() under the hood and uses the resulting self.qimage.

So the following code would have made a round-trip from QImage to Pixmap back to QImage, so why not just use self.qimage directly?

pm = QPixmap.fromImage(self.qimage)
im = pm.toImage()

In my opinion, the includeBorder option makes the ImageItem API non-orthogonal. Why allow the user to choose to have a saved image that is different from what is shown on screen?

It is easy to create a function outside of the library that serves one's particular need (e.g. one particular dtype) and that's what I do. But to make a general function, that's difficult because now you have to handle all corner cases.

[ntjess]

@pijyoi

getPixmap() calls render() under the hood and uses the resulting self.qimage.

So the following code would have made a round-trip from QImage to Pixmap back to QImage, so why not just use self.qimage directly?

pm = QPixmap.fromImage(self.qimage)
im = pm.toImage()

That sounds like a better way of doing things.

In my opinion, the includeBorder option makes the ImageItem API non-orthogonal. Why allow the user to choose to have a saved image that is different from what is shown on screen?

I wasn't sure if old behavior of save not including the border should be maintained if desired. Otherwise, I would agree it should be included without parameters. I would say the same thing for opacity, but for the same reason I didn't know if backwards behavior should be preserved somehow.

It is easy to create a function outside of the library that serves one's particular need (e.g. one particular dtype) and that's what I do. But to make a general function, that's difficult because now you have to handle all corner cases.

Sounds good, does this look better? Tests for I think the dtypes and shapes you mentioned

from pathlib import Path

import pyqtgraph as pg
import numpy as np
from pyqtgraph.Qt import QtGui, QtCore, QtWidgets
from skimage.data import chelsea
from skimage.util import *

pg.mkQApp()

class NewImageItem(pg.ImageItem):
  def save(self, fileName, *args):
    if self._renderRequired:
      self.render()
    # Don't make the pixmap if you don't have to :)
    im = QtGui.QImage(self.qimage)
    im.fill(QtCore.Qt.transparent)
    painter = QtGui.QPainter(im)
    oldBorder = self.border
    painter.setOpacity(self.opacity())
    self.paint(painter)
    self.setBorder(oldBorder)
    painter.end()
    ret = im.save(fileName, *args)
    assert ret

outs = Path('./images')
outs.mkdir(exist_ok=True)
app = pg.mkQApp()
ii = NewImageItem()
ii.setBorder(pg.mkPen('r', width=5))
data = chelsea()
data = np.dstack([data, np.full(data.shape[:2] + (1,), 255, dtype=data.dtype)])
converts = [img_as_float32, img_as_bool, img_as_int, img_as_uint, img_as_ubyte, img_as_float]
shapes = [(), (slice(0, 3),), (0,), (slice(0, 1),)]
for shape in shapes:
  for cvt in converts:
    curData = cvt(data)
    curSlice = (...,) + shape
    curData = curData[curSlice]
    ii.setOpacity(float(np.random.random(1)))
    ii.setImage(curData)
    ii.save(str(outs/f'{curData.shape} - {cvt.__name__} - opacity {ii.opacity()}.png'))

[pijyoi]

Looking at the documentation, for opacity: https://doc.qt.io/qt-5/qgraphicsitem.html#setOpacity It seems that if you really want to save what appears on screen, you would need effectiveOpacity() instead.

So if we go back to your original premise:

ii = pg.ImageItem()
data = np.random.randint(256, size=(500,500,3), dtype=np.uint8)
ii.setImage(data)
ii.setOpacity(0.5)

is not functionally equivalent to:

ii = pg.ImageItem()
data = np.random.randint(256, size=(500,500,4), dtype=np.uint8)
ii.setImage(data)
data[..., 3] = 128

[ntjess]

Nice catch. It looks like a simple fix though, I just have to change the relevant line to painter.setOpacity(self.effectiveOpacity()), right? It seems to be working in my example suite.

I also removed the border lines since of course they were from when I was considering preserving old behavior. For completeness:

class NewImageItem(pg.ImageItem):
  def save(self, fileName, *args):
    if self._renderRequired:
      self.render()
    # Don't make the pixmap if you don't have to :)
    im = QtGui.QImage(self.qimage)
    # I can also run timing analysis to see if it's faster to use
    # im = QtGui.QImage(im.size(), im.format())
    im.fill(QtCore.Qt.transparent)
    painter = QtGui.QPainter(im)
    painter.setOpacity(self.effectiveOpacity())
    self.paint(painter)
    # Maybe "end" isn't necessary?
    painter.end()
    ret = im.save(fileName, *args)
    assert ret

[pijyoi]

So the difference between ImageItem::save() and NewImageItem::save() is that the former saves the image that has passed through pyqtgraph library processing (downsampling, contrast stretching, colormapping, etc), whereas the latter wants to save what has further gone through Qt library painting processing.

That seems like two different plausible use-cases. i.e. the old behavior should be retained and the new behavior should only be enabled with a parameter, or even provided as a separate method entirely. @ntjess, wouldn't your needs be already addressed with your sub-classed class? Perhaps others would like to weigh in.

[ntjess]

@pijyoi As I understood it, save was designed to put a visual representation of the ImageItem to disk (or to bytes/clipboard if that is of interest). The current implementation of save does not do this faithfully: 1) the image border is not represented, and 2) any widget opacity is discarded. I would consider those bugs under the current description of save. The purpose of NewImageItem::save was to fulfill both missing objectives. If there is an easier way, that sounds great -- but this was the motivation for any changes I described.

[ntjess]

@pijyoi bumping the thread. I am fine with closing this issue if the current save behavior is considered complete, but I would appreciate some brief feedback on my previous response before then. Thanks!

[pijyoi]

@ntjess given that you have already invested time modifying the code to fix what you perceive as a bug, you could raise a PR for a review.

I don't use save myself, but my view is that the existing behaviour, whether fulfilling its documented API or not, would have already become the de facto API.

https://www.hyrumslaw.com/

[j9ac9k]

FWIW I do agree that opacity should be included as part of the .save() functionality; I can also see an issue w/ changing default behavior as pijyoi points out. I would be good with having an includeOpacity named argument to ImageItem.save, which default is to False (to preserve current behavior), but we can put in the doc-strings that it will change to True in future versions...

[ntjess]

@j9ac9k Right. As you can see in my earlier comment I suggested the same thing for the image border, with False defaults for backwards compatibility. The reply was that save should represent what's on the screen. It doesn't matter to me either way, I'd just put in whatever is easier for a PR review.

[j9ac9k]

I somehow missed pijyoi's post:

So the difference between ImageItem::save() and NewImageItem::save() is that the former saves the image that has passed through pyqtgraph library processing (downsampling, contrast stretching, colormapping, etc), whereas the latter wants to save what has further gone through Qt library painting processing.

This is a compelling argument to preserving existing behavior, but I believe we should have the behavior you describe available; I don't think it should be a new method (save is pretty generic sounding) but should likely have a very detailed doc-string detailing what the existing behavior looks like (more so than what's currently there).