How to update a canvas.Image.Image from a different goroutine?

[rgl]

Currently, I'm creating a canvas.Image image in the main goroutine as:

a := app.New()
w := a.NewWindow("Cameras")
cameraImage := canvas.NewImageFromImage(nil)
cameraImage.FillMode = canvas.ImageFillContain
cameraImage.ScaleMode = canvas.ImageScaleFastest
cameraImage.SetMinSize(fyne.NewSize(320, 240))
w.SetContent(cameraImage)
w.ShowAndRun()

And in another goroutine, I'm updating it as:

img, _, err := image.Decode(bytes.NewReader(data))
cameraImage.Image = img // TODO should we really touch the Image property without a lock?
cameraImage.Refresh()

Although that seems to work. it does not feel correct to set that variable without a lock.

Is that the correct way to modify the image?

Should a canvas.Image.SetImage function exist?

[metal3d]

As you said, changing variables content in another goroutine needs to lock the memory (with sync.Mutex for example). The fact is that if we should create a setter for all the properties, we need to create a lot of methods and make all the properties as private.

Actually, Fyne let you manage the memory as you want, and that's right for a lot of others packages. So to respond the last quest, in my opinion, it's no 😄

For your example, I would create a custom widget (CamerView) used in both windows, this widget could implement SetImage()with a lock.

[rgl]

Not sure if I follow. Please be aware that I'm a Fyne newbie.

It seems you've shifted the need for a lock to the CreateRenderer implementation instead?

I'm not familiar with CreateRenderer, but isn't that only called once and cached forever? Which might mean, that new images will never be displayed.

Also, it seems this will create a new canvas.Image every-time a new camera frame is set (which means, more garbage for the garbage collector to collect)?

Also, instead of creating a new widget, why not use one of the existing ones? like widget.Card which has a SetImage function?

[metal3d]

What I mean is that in your case, you can create a "custom widget" by implementing the fyne.Widget interface. In this case, the widget wraps the CanvaObject (image) and give it a method to change the image with a lock.

The CreateRender() is internally called by Fyne to get a renderer. You don't need to call it, use the CameraView like any other widget (Label, Entry, Button...)

You can then create the CameraView with NewCameraView and place it wherever you want. Each time the camera returns an image (image.Image) you can send it to the widget with SetImage():

// get a new widget
camview := NewCameraView(nil)

// launch the goroutine and set the image as you need
go func(){
   // e.g. if getNextFrame() returns a chan of image.Image
   for im := range getNextFrame() {
     camview.SetImage(im)
   }
}()


// add the widget in your app window, or in any container
window.SetContent(camview)

Of course, my example needs checks for "nil" image.

[rgl]

I understood your use of the CameraView widget. What I did not understood is what/when a lock must be held to write and read the c.img variable. Especially, since you just have shifted that problem into the CameraView widget, leaving one access to c.img be made without a lock being held.

From my viewpoint, any code that uses c.img must do it properly, regardless of what/when calls it (e.g. does not matter if called internally by Fyne; any code must obey the same rules).

[metal3d]

It's a very good question. Actually, I guess that it's not a big deal to ensure IO on image. See this example:

// Ensure CamView implements fyne.Widget
var _ fyne.Widget = (*CamView)(nil)

// CamView is a widget that displays an image.
type CamView struct {
	widget.BaseWidget
	locker sync.Mutex
	img    *canvas.Image
}

// NewCamView creates a new CamView widget. It implements fyne.Widget.
func NewCamView(im image.Image) *CamView {
	c := &CamView{
		locker: sync.Mutex{},
		img:    canvas.NewImageFromImage(im),
	}
	c.SetImage(im)
	c.ExtendBaseWidget(c)
	return c
}

// SetImage sets the image to display.
func (c *CamView) SetImage(im image.Image) {
	if im == nil {
		return
	}
	c.locker.Lock()
	defer c.Refresh() // not mandatory, you can remove this if you want to call it yourself
	defer c.locker.Unlock()
	c.img.Image = im
	c.img.FillMode = canvas.ImageFillContain
	c.img.ScaleMode = canvas.ImageScalePixels
	c.img.SetMinSize(fyne.NewSize(100, 100))
}

// Refresh refreshes the widget (the image).
func (c *CamView) Refresh() {
	if c.img == nil {
		return
	}
	c.locker.Lock()
	defer c.locker.Unlock()
	c.img.Refresh()
}

// GetImage returns the image used by this widget.
func (c *CamView) GetImage() *canvas.Image {
	if c.img == nil {
		return &canvas.Image{}
	}
	c.locker.Lock()
	defer c.locker.Unlock()
	return c.img

}

// CreateRenderer is a private method to Fyne which links this widget to its renderer.
//
// implement fyne.Widget
func (c *CamView) CreateRenderer() fyne.WidgetRenderer {
	return widget.NewSimpleRenderer(container.NewStack(c.GetImage()))
}

This should work.

• SetImage locks the memory before setting the image
• Refresh is called by SetImage(), or you can call it yourself from your main function

Fyne calls Refresh() to display the widget, this should then lock the memory.

[metal3d]

Just in case of: https://gist.github.com/metal3d/d6e295bef424ad18f801bf7f026f0019

Using a V4L package. The stream is very smooth on my laptop.

[metal3d]

To propose something that "works" in my laptop, here is a gist: https://gist.github.com/metal3d/d6e295bef424ad18f801bf7f026f0019

The proposed example in the conversation was a good start, but the gist is tested.

The widget correctly set the image with lock. Fyne does not read the image until you don't call Refresh(). This method is overridden to lock the memory and avoid SetImage() to write in memory at the same time.

It is, theoretically, OK.

[rgl]

Thanks for looking into this!

But I'm afraid the end result seems too complex for me, a user of Fyne.

As an alternative, I've tried to use the existing widget.NewCard as:

	cameraCard := widget.NewCard(devices[0].Card, "", nil)
	cameraImage := canvas.NewImageFromImage(nil)
	cameraImage.SetMinSize(fyne.NewSize(320, 240))
	cameraCard.SetImage(cameraImage)

And in the streaming goroutine, call Card.SetImage:

				cameraImage := canvas.NewImageFromImage(img)
				cameraImage.FillMode = canvas.ImageFillContain
				cameraImage.ScaleMode = canvas.ImageScaleFastest
				cameraImage.SetMinSize(fyne.NewSize(320, 240))
				cameraCard.SetImage(cameraImage)

But, looking at the widget.NewCard.SetImage implementation, I'm not entirely sure its doing the right thing.

So, either Fyne does not support using goroutines and all changes must be done from the main goroutine (like other ui toolkits, which only allow uses from the ui thread). Or, not all the Fyne widgets seem to be implemented correctly. Or, I'm seeing this all wrong.

[andydotxyz]

It is possible that the setters on card are not locking correctly, if you reckon it's not safe.

As mentioned we do want to make the locking easier, if you want to manipulate fields manually and also control access to avoid possible data races.

[Jacalz]

The idea with Fyne is basically that you shouldn't have to call stuff from the main goroutine (we don't even give developers access to do so). If we lack locking in some places we should improve that

[andydotxyz]

Thanks for this @metal3d - the toolkit could probably do more to make this easier.
I had played with the idea of exposing a widget memory lock in the past but it did not complete.
Perhaps that is exactly what is needed for more expert usage and those keen to eliminate possible races?

[metal3d]

I think that we only need to ensure the canvas.Image with a setter. But, IMHO, changing image frequently needs to override the widget like in my (not well made) example. If I'm not wrong, Fyne doesn't read the variables while the Refresh() function is not called.

Should Fyne take care of memory locks while it commonly not needed, or leave us to lock when needed (with overrides)?

It's a very interesting question. And thanks to @rgl to spot this 🙂

[dweymouth]

Personally I'd be in favor of adding setters for widget properties. Keeps the locking complexity hidden from the user, and the setters would ensure that no data race will occur. Leave the exported properties but document that setting them directly is only for constructors (eg when making extended widgets). I also think this is a low-priority issue. I have a Fyne app that uses images heavily and updates them from goroutines by just setting the .Image property and I haven't hit the race condition (that I know of).

[metal3d]

The fact is that the canvas.Image is not a widget. It's a fine.CanvasObject and this is why I proposed to create a custom widget to implement the setter. Actually, it could be enough to simply propose a widget.Image. isn't it ? Le mar. 5 sept. 2023, 18:18, Drew Weymouth ***@***.***> a écrit :

…

Personally I'd be in favor of adding setters for widget properties. Keeps the locking complexity hidden from the user, and the setters would ensure that no data race will occur. Leave the exported properties but document that setting them directly is only for constructors (eg when making extended widgets). I also think this is a low-priority issue. I have a Fyne app that uses images heavily and updates them from goroutines by just setting the .Image property and I haven't hit the race condition (that I know of). — Reply to this email directly, view it on GitHub <#4213 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAYN4FGVP2HUYM6RW7AENLXY5GEPANCNFSM6AAAAAA4IXTWVU> . You are receiving this because you were mentioned.Message ID: ***@***.***>