[gold] mem_diffstore.go: Stop writing diff images to disk.
Bug: skia:9287
Change-Id: I3c00e1b9bcbe042033c5419c54755cd5d9325619
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/234361
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
diff --git a/golden/go/diff/metrics.go b/golden/go/diff/metrics.go
index 9d083ba..0ede1ea 100644
--- a/golden/go/diff/metrics.go
+++ b/golden/go/diff/metrics.go
@@ -39,11 +39,11 @@
return diffMetricIds
}
-// DefaultDiffFn implements the DiffFn function type. Calculates the basic
-// image difference along with custom diff metrics.
-func DefaultDiffFn(leftImg *image.NRGBA, rightImg *image.NRGBA) (interface{}, *image.NRGBA) {
+// DefaultDiffFn implements the DiffFn function type. It computes
+// and returns the diff metrics between two given images.
+func DefaultDiffFn(leftImg *image.NRGBA, rightImg *image.NRGBA) interface{} {
defer metrics2.FuncTimer().Stop()
- ret, diffImg := PixelDiff(leftImg, rightImg)
+ ret, _ := PixelDiff(leftImg, rightImg)
// Calculate the metrics.
diffs := make(map[string]float32, len(diffMetricIds))
@@ -52,7 +52,7 @@
}
ret.Diffs = diffs
- return ret, diffImg
+ return ret
}
// combinedDiffMetric returns a value in [0, 1] that represents how large
diff --git a/golden/go/diffstore/mapper/disk_mapper/disk_mapper.go b/golden/go/diffstore/mapper/disk_mapper/disk_mapper.go
index d846ede..bf1d954 100644
--- a/golden/go/diffstore/mapper/disk_mapper/disk_mapper.go
+++ b/golden/go/diffstore/mapper/disk_mapper/disk_mapper.go
@@ -17,7 +17,7 @@
// DiskMapper implements the Mapper interface.
// It uses diff.DiffMetrics as the Gold diff metric.
-// It stores the images and diffs on disk using a two
+// It stores the images on disk using a two
// level radix prefix (i.e. for digest "abcdefg.png", the
// image will be in ab/cd/abcdefg.png). The use of the radix
// allows us to work around limits in Linux of how many files
@@ -33,19 +33,10 @@
}
// DiffFn implements the DiffStoreMapper interface.
-func (g *DiskMapper) DiffFn(left *image.NRGBA, right *image.NRGBA) (interface{}, *image.NRGBA) {
+func (g *DiskMapper) DiffFn(left *image.NRGBA, right *image.NRGBA) interface{} {
return diff.DefaultDiffFn(left, right)
}
-// SplitDiffID implements the DiffStoreMapper interface.
-func (g *DiskMapper) DiffPath(left, right types.Digest) string {
- // Get the diff ID and the left imageID.
- diffID := mapper.DiffID(left, right)
- imagePath := fmt.Sprintf("%s.%s", diffID, imgExtension)
-
- return fileutil.TwoLevelRadixPath(imagePath)
-}
-
// ImagePaths implements the DiffStoreMapper interface.
func (g *DiskMapper) ImagePaths(imageID types.Digest) (string, string) {
gsPath := fmt.Sprintf("%s.%s", imageID, imgExtension)
diff --git a/golden/go/diffstore/mapper/disk_mapper/disk_mapper_test.go b/golden/go/diffstore/mapper/disk_mapper/disk_mapper_test.go
index b39c153..94466be 100644
--- a/golden/go/diffstore/mapper/disk_mapper/disk_mapper_test.go
+++ b/golden/go/diffstore/mapper/disk_mapper/disk_mapper_test.go
@@ -11,26 +11,13 @@
)
const (
- // Arbitrary MD5 digests
+ // Arbitrary MD5 digest.
imgOne = types.Digest("098f6bcd4621d373cade4e832627b4f6")
- imgTwo = types.Digest("1660f0783f4076284bc18c5f4bdc9608")
-
- exampleDiffId = "098f6bcd4621d373cade4e832627b4f6-1660f0783f4076284bc18c5f4bdc9608"
// PNG extension.
png = ".png"
)
-func TestDiffPath(t *testing.T) {
- unittest.SmallTest(t)
-
- dm := New(&testutils.DummyDiffMetrics{})
-
- actualDiffPath := dm.DiffPath(imgOne, imgTwo)
- expectedPath := filepath.Join("09", "8f", exampleDiffId+png)
- assert.Equal(t, expectedPath, actualDiffPath)
-}
-
func TestImagePaths(t *testing.T) {
unittest.SmallTest(t)
diff --git a/golden/go/diffstore/mapper/mapper.go b/golden/go/diffstore/mapper/mapper.go
index 757707b..667af1d 100644
--- a/golden/go/diffstore/mapper/mapper.go
+++ b/golden/go/diffstore/mapper/mapper.go
@@ -10,28 +10,23 @@
)
// Mapper is the interface to define how the diff metric between two images
-// is calculated and defines how those diffs are stored on disk.
-// TODO(kjlubick): It might be nice to have Mapper just focus on the
-// diff metric and have a different interface for the disk storing.
-// Or make the interface just serve the bytes (if possible).
+// is calculated.
type Mapper interface {
// LRUCodec defines the Encode and Decode functions to serialize/deserialize
// instances of the diff metrics returned by the DiffFn function below.
util.LRUCodec
- // DiffFn calculates the different between two given images and returns a
- // difference image. The type underlying interface{} is the input and output
- // of the LRUCodec above. It is also what is returned by the Get(...) function
- // of the DiffStore interface.
- DiffFn(*image.NRGBA, *image.NRGBA) (interface{}, *image.NRGBA)
-
- // DiffPath returns the local file path for the diff image of two images.
- // This path is used to store the diff image on disk and serve it over HTTP.
- DiffPath(left, right types.Digest) string
+ // DiffFn computes and returns the diff metrics between two given images.
+ // The type underlying interface{} is the input and output of the LRUCodec
+ // above. It is also what is returned by the Get(...) function of the
+ // DiffStore interface.
+ DiffFn(*image.NRGBA, *image.NRGBA) interface{}
// ImagePaths returns the storage paths for a given image ID. The first return
// value is the local file path used to store the image on disk and serve it
// over HTTP. The second return value is the GCS path (not including the bucket).
+ // TODO(kjlubick): It might be nice to have Mapper just focus on the
+ // diff metric and have a different interface for the disk storing.
ImagePaths(id types.Digest) (string, string)
}
diff --git a/golden/go/diffstore/mem_diffstore.go b/golden/go/diffstore/mem_diffstore.go
index 2d4dfaa..3c2854d 100644
--- a/golden/go/diffstore/mem_diffstore.go
+++ b/golden/go/diffstore/mem_diffstore.go
@@ -1,7 +1,6 @@
package diffstore
import (
- "bytes"
"fmt"
"math"
"net/http"
@@ -56,10 +55,7 @@
// baseDir contains the root directory of where all data are stored.
baseDir string
- // localDiffDir is the directory where diff images are written to.
- localDiffDir string
-
- // diffMetricsCache caches and calculates diff metrics and images.
+ // diffMetricsCache caches and calculates diff metrics.
diffMetricsCache rtcache.ReadThroughCache
// imgLoader fetches and caches images.
@@ -94,12 +90,6 @@
return nil, skerr.Fmt("Could not make image directory %s: %s", imgPath, err)
}
- diffPath := filepath.Join(baseDir, DEFAULT_DIFFIMG_DIR_NAME)
- difDir, err := fileutil.EnsureDirExists(diffPath)
- if err != nil {
- return nil, skerr.Fmt("Could not make diff image directory %s: %s", diffPath, err)
- }
-
// Set up image retrieval, caching and serving.
imgLoader, err := NewImgLoader(client, baseDir, imgDir, gsBucketNames, gsImageBaseDir, imageCacheCount, m)
if err != nil {
@@ -113,7 +103,6 @@
ret := &MemDiffStore{
baseDir: baseDir,
- localDiffDir: difDir,
imgLoader: imgLoader,
metricsStore: mStore,
mapper: m,
@@ -389,23 +378,16 @@
}
// We are guaranteed to have two images at this point.
- diffMetrics, diffImg := d.mapper.DiffFn(imgs[0], imgs[1])
+ diffMetrics := d.mapper.DiffFn(imgs[0], imgs[1])
- // Encode the result image and save it to disk. If encoding causes an error
- // we return an error.
- var buf bytes.Buffer
- if err = common.EncodeImg(&buf, diffImg); err != nil {
- return nil, err
- }
-
- // Save the diffMetrics and the diffImage.
- d.saveDiffInfoAsync(id, leftDigest, rightDigest, diffMetrics, buf.Bytes())
+ // Save the diffMetrics.
+ d.saveDiffMetricsAsync(id, leftDigest, rightDigest, diffMetrics)
return diffMetrics, nil
}
-// saveDiffInfoAsync saves the given diff information to disk asynchronously.
-func (d *MemDiffStore) saveDiffInfoAsync(diffID string, leftDigest, rightDigest types.Digest, diffMetrics interface{}, imgBytes []byte) {
- d.wg.Add(2)
+// saveDiffMetricsAsync saves the given diff metrics to disk asynchronously.
+func (d *MemDiffStore) saveDiffMetricsAsync(diffID string, leftDigest, rightDigest types.Digest, diffMetrics interface{}) {
+ d.wg.Add(1)
d.maxGoRoutinesCh <- true
go func() {
defer func() {
@@ -416,20 +398,6 @@
sklog.Errorf("Error saving diff metric: %s", err)
}
}()
-
- d.maxGoRoutinesCh <- true
- go func() {
- defer func() {
- d.wg.Done()
- <-d.maxGoRoutinesCh
- }()
-
- // Get the local file path using the mapper and save the diff image there.
- localDiffPath := d.mapper.DiffPath(leftDigest, rightDigest)
- if err := common.SaveFilePath(filepath.Join(d.localDiffDir, localDiffPath), bytes.NewBuffer(imgBytes)); err != nil {
- sklog.Error(err)
- }
- }()
}
// Returns all combinations of leftDigests and rightDigests using the given
diff --git a/golden/go/diffstore/mem_diffstore_test.go b/golden/go/diffstore/mem_diffstore_test.go
index bf17dce..ab625d1 100644
--- a/golden/go/diffstore/mem_diffstore_test.go
+++ b/golden/go/diffstore/mem_diffstore_test.go
@@ -3,12 +3,10 @@
import (
"fmt"
"path"
- "path/filepath"
"sort"
"testing"
assert "github.com/stretchr/testify/require"
- "go.skia.org/infra/go/fileutil"
"go.skia.org/infra/go/testutils"
"go.skia.org/infra/go/testutils/unittest"
"go.skia.org/infra/go/tiling"
@@ -103,7 +101,7 @@
}
}
ti.Stop()
- testDiffs(t, baseDir, memDiffStore, digests, digests, foundDiffs)
+ testDiffs(t, memDiffStore, digests, digests, foundDiffs)
// Get the results directly and make sure they are correct.
digests = testDigests[1][:TEST_N_DIGESTS]
@@ -115,18 +113,16 @@
foundDiffs[oneDigest] = found
}
ti.Stop()
- testDiffs(t, baseDir, memDiffStore, digests, digests, foundDiffs)
+ testDiffs(t, memDiffStore, digests, digests, foundDiffs)
}
-func testDiffs(t *testing.T, baseDir string, diffStore *MemDiffStore, leftDigests, rightDigests types.DigestSlice, result map[types.Digest]map[types.Digest]interface{}) {
+func testDiffs(t *testing.T, diffStore *MemDiffStore, leftDigests, rightDigests types.DigestSlice, result map[types.Digest]map[types.Digest]interface{}) {
diffStore.sync()
for _, left := range leftDigests {
for _, right := range rightDigests {
if left != right {
_, ok := result[left][right]
assert.True(t, ok, fmt.Sprintf("left: %s, right:%s", left, right))
- diffPath := diffStore.mapper.DiffPath(left, right)
- assert.True(t, fileutil.FileExists(filepath.Join(diffStore.localDiffDir, diffPath)), fmt.Sprintf("Could not find %s", diffPath))
}
}
}
