Replies: 5 comments 17 replies
-
|
As you said, changing variables content in another goroutine needs to lock the memory (with 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 ( |
Beta Was this translation helpful? Give feedback.
-
|
I understood your use of the From my viewpoint, any code that uses |
Beta Was this translation helpful? Give feedback.
-
|
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.
Fyne calls |
Beta Was this translation helpful? Give feedback.
-
|
Just in case of: https://gist.github.com/metal3d/d6e295bef424ad18f801bf7f026f0019 Using a V4L package. The stream is very smooth on my laptop. |
Beta Was this translation helpful? Give feedback.
-
|
@metal3d I like your solution, but that works only if you control the enclosing widget, right? Wherever you want an image, you instead use a I am facing a similar problem as OP, but it is with |
Beta Was this translation helpful? Give feedback.
-
|
There should be no problem here - either call the setter method or update fields and then call the object's Refresh method. That's how it is designed to work and I've not come across a case where it doesn't. |
Beta Was this translation helpful? Give feedback.
-
|
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 It is, theoretically, OK. |
Beta Was this translation helpful? Give feedback.
-
|
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 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 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 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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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 |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for this @metal3d - the toolkit could probably do more to make this easier. |
Beta Was this translation helpful? Give feedback.
-
|
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 🙂 |
Beta Was this translation helpful? Give feedback.
-
|
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). |
Beta Was this translation helpful? Give feedback.
-
|
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:
***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
Just as an FYI to this thread 2.6 will slightly adjust the threading model but for now all Fyne code should just work from any goroutine. |
Beta Was this translation helpful? Give feedback.
-
|
@andydotxyz maybe I'm using the library incorrectly, but I see data races updating a card from a goroutine. As a test, I am creating a scrollable grid view of cards. Each card is responsible for setting it's If I make the call to
When I load the image in a goroutine (as in the code above), the contents does not change from the original black rectangle:
I can see that there is indeed a race (below). The only thing I can think of is that I am loading the image into |
Beta Was this translation helpful? Give feedback.
-
|
It's more likely that you've found a bug in the Card widget than that it's a problematic race. |
Beta Was this translation helpful? Give feedback.
-
|
We are aware that there are races in v2.5.x, partly due to various issues around how the threading model works at the moment. That is what we are trying to fix in v2.6.0 but like Andrew says, having no rendering at all due to races seems unlikely. You might get odd behaviour but getting no data at all is not something we ever have observed as far as I know. |
Beta Was this translation helpful? Give feedback.
-
|
I have filed #5318 for you to play with 😀 |
Beta Was this translation helpful? Give feedback.
-
Currently, I'm creating a
canvas.Imageimage in the main goroutine as:And in another goroutine, I'm updating it as:
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.SetImagefunction exist?Beta Was this translation helpful? Give feedback.
All reactions