Unfortunately, NewImageOp does need to make a copy, see
gioui.org/commit/74407a50d598bfd27e8f8e48b6832cc5df04de77.
There are three reasons to make a copy, two important and
one less so:
First, the ImageOp.src field is used as a key in an image=>texture map to
quickly determine whether we can reuse an existing texture for an image. There
is no efficient way to know whether the user has mutated an image, which should
replace the cached texture.
Second, even if we somehow detected image mutations, letting the program use
the image contents while the GPU goroutine is accessing it leads to race
conditions.
Third, and less importantly, sometimes the image contents is not in a GPU
friendly format and a copy is needed anyway.
That said, I might have missed a better design for efficient drawing of images.
Please let me know.
On Thu Nov 7, 2019 at 3:32 PM aarzilli wrote:
> ---> op/paint/paint.go | 28 ++++++++++++++++++----------> 1 file changed, 18 insertions(+), 10 deletions(-)> > diff --git a/op/paint/paint.go b/op/paint/paint.go> index 4b71d8c..68d9ec3 100644> --- a/op/paint/paint.go> +++ b/op/paint/paint.go> @@ -41,18 +41,26 @@ func NewImageOp(src image.Image) ImageOp {> uniform: true,> color: col,> }> - default:> - sz := src.Bounds().Size()> - // Copy the image into a GPU friendly format.> - dst := image.NewRGBA(image.Rectangle{> - Max: sz,> - })> - draw.Draw(dst, src.Bounds(), src, image.Point{}, draw.Src)> - return ImageOp{> - src: dst,> - size: sz,> + case *image.RGBA:> + if src.Bounds().Min == (image.Point{}) {> + sz := src.Bounds().Size()> + return ImageOp{> + src: src,> + size: sz,> + }> }> }> +> + sz := src.Bounds().Size()> + // Copy the image into a GPU friendly format.> + dst := image.NewRGBA(image.Rectangle{> + Max: sz,> + })> + draw.Draw(dst, src.Bounds(), src, image.Point{}, draw.Src)> + return ImageOp{> + src: dst,> + size: sz,> + }> }> > func (i ImageOp) Size() image.Point {> -- > 2.17.1
On Thu, Nov 07, 2019 at 04:21:13PM +0100, Elias Naur wrote:
> Unfortunately, NewImageOp does need to make a copy, see> gioui.org/commit/74407a50d598bfd27e8f8e48b6832cc5df04de77.> > There are three reasons to make a copy, two important and> one less so:> > First, the ImageOp.src field is used as a key in an image=>texture map to> quickly determine whether we can reuse an existing texture for an image. There> is no efficient way to know whether the user has mutated an image, which should> replace the cached texture.
I don't think this introduces any expectation that the ImageOp will reflect
changes made to the source image.Image. It didn't before, it doesn't now.
Also, why store the texture id in a map instead of putting it in a private
field of ImageOp?
> Second, even if we somehow detected image mutations, letting the program use> the image contents while the GPU goroutine is accessing it leads to race> conditions.
That would be the same problem that exists with op.Ops. It is a valid
concern and should be weighted against making one extra copy here, so feel
free to disregard this patch if you feel that this weights too much against
this.
> > Third, and less importantly, sometimes the image contents is not in a GPU> friendly format and a copy is needed anyway.> > That said, I might have missed a better design for efficient drawing of images.> Please let me know.> > On Thu Nov 7, 2019 at 3:32 PM aarzilli wrote:> > ---> > op/paint/paint.go | 28 ++++++++++++++++++----------> > 1 file changed, 18 insertions(+), 10 deletions(-)> > > > diff --git a/op/paint/paint.go b/op/paint/paint.go> > index 4b71d8c..68d9ec3 100644> > --- a/op/paint/paint.go> > +++ b/op/paint/paint.go> > @@ -41,18 +41,26 @@ func NewImageOp(src image.Image) ImageOp {> > uniform: true,> > color: col,> > }> > - default:> > - sz := src.Bounds().Size()> > - // Copy the image into a GPU friendly format.> > - dst := image.NewRGBA(image.Rectangle{> > - Max: sz,> > - })> > - draw.Draw(dst, src.Bounds(), src, image.Point{}, draw.Src)> > - return ImageOp{> > - src: dst,> > - size: sz,> > + case *image.RGBA:> > + if src.Bounds().Min == (image.Point{}) {> > + sz := src.Bounds().Size()> > + return ImageOp{> > + src: src,> > + size: sz,> > + }> > }> > }> > +> > + sz := src.Bounds().Size()> > + // Copy the image into a GPU friendly format.> > + dst := image.NewRGBA(image.Rectangle{> > + Max: sz,> > + })> > + draw.Draw(dst, src.Bounds(), src, image.Point{}, draw.Src)> > + return ImageOp{> > + src: dst,> > + size: sz,> > + }> > }> > > > func (i ImageOp) Size() image.Point {> > -- > > 2.17.1
On Thu Nov 7, 2019 at 5:01 PM aarzilli wrote:
> On Thu, Nov 07, 2019 at 04:21:13PM +0100, Elias Naur wrote:> > Unfortunately, NewImageOp does need to make a copy, see> > gioui.org/commit/74407a50d598bfd27e8f8e48b6832cc5df04de77.> > > > There are three reasons to make a copy, two important and> > one less so:> > > > First, the ImageOp.src field is used as a key in an image=>texture map to> > quickly determine whether we can reuse an existing texture for an image. There> > is no efficient way to know whether the user has mutated an image, which should> > replace the cached texture.> > I don't think this introduces any expectation that the ImageOp will reflect> changes made to the source image.Image. It didn't before, it doesn't now.> Also, why store the texture id in a map instead of putting it in a private> field of ImageOp?>
Good idea.
> > Second, even if we somehow detected image mutations, letting the program use> > the image contents while the GPU goroutine is accessing it leads to race> > conditions.> > That would be the same problem that exists with op.Ops. It is a valid> concern and should be weighted against making one extra copy here, so feel> free to disregard this patch if you feel that this weights too much against> this.>
That used to be true that op.Ops would not be touched after returning from e.Frame.
I'll fix that.
That leaves your patch being an acceptable optimization, thanks. One thing: I believe
you also need to check the image.RGBA.Stride field.
-- elias
On Thu Nov 7, 2019 at 5:25 PM Elias Naur wrote:
> On Thu Nov 7, 2019 at 5:01 PM aarzilli wrote:> > On Thu, Nov 07, 2019 at 04:21:13PM +0100, Elias Naur wrote:> > > Unfortunately, NewImageOp does need to make a copy, see> > > gioui.org/commit/74407a50d598bfd27e8f8e48b6832cc5df04de77.> > > > > > There are three reasons to make a copy, two important and> > > one less so:> > > > > > First, the ImageOp.src field is used as a key in an image=>texture map to> > > quickly determine whether we can reuse an existing texture for an image. There> > > is no efficient way to know whether the user has mutated an image, which should> > > replace the cached texture.> > > > I don't think this introduces any expectation that the ImageOp will reflect> > changes made to the source image.Image. It didn't before, it doesn't now.> > Also, why store the texture id in a map instead of putting it in a private> > field of ImageOp?> > > > Good idea.> > > > Second, even if we somehow detected image mutations, letting the program use> > > the image contents while the GPU goroutine is accessing it leads to race> > > conditions.> > > > That would be the same problem that exists with op.Ops. It is a valid> > concern and should be weighted against making one extra copy here, so feel> > free to disregard this patch if you feel that this weights too much against> > this.> > > > That used to be true that op.Ops would not be touched after returning from e.Frame.> I'll fix that.> > That leaves your patch being an acceptable optimization, thanks. One thing: I believe> you also need to check the image.RGBA.Stride field.> > -- elias
All done, and your patch is merged with the stride check in
https://gioui.org/commit/454b2264048f39952fb7de8f853c8545f171ceeb
Remember to sign off your future patches, and send them to ~eliasnaur/gio-patches@lists.sr.ht,
which was recently created because of the increased patch volume :)