~eliasnaur/gio

op/paint: NewImageOp does not need to make copies of RGBA images v1 PROPOSED

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
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~eliasnaur/gio/patches/9010/mbox | git am -3
Learn more about email & git

[PATCH] op/paint: NewImageOp does not need to make copies of RGBA images Export this patch

aarzilli
---
 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
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
View this thread in the archives