From 5429eea7e4cec8d6f6604372d683e4dbe4fc9c90 Mon Sep 17 00:00:00 2001 From: Peter Nemere Date: Wed, 27 Mar 2024 08:07:05 +1000 Subject: [PATCH 1/2] More logging to diagnose why beam locations are not saved correctly for uploaded datasets. Fixed some error handling too --- api/dataimport/internal/output/output.go | 14 +++++++++++--- api/quantification/create.go | 7 +------ api/quantification/importCSV.go | 5 +---- api/ws/handlers/element-set.go | 6 ++---- api/ws/handlers/expression-group.go | 5 +---- api/ws/handlers/expression.go | 5 +---- api/ws/handlers/module.go | 5 +---- api/ws/handlers/roi.go | 5 +---- api/ws/handlers/screen-configuration.go | 5 +---- api/ws/wsHelpers/ownership.go | 4 ++-- core/beamLocation/experimentFileToDB.go | 2 ++ 11 files changed, 24 insertions(+), 39 deletions(-) diff --git a/api/dataimport/internal/output/output.go b/api/dataimport/internal/output/output.go index 0d56d79d..909aa498 100644 --- a/api/dataimport/internal/output/output.go +++ b/api/dataimport/internal/output/output.go @@ -318,6 +318,10 @@ func (s *PIXLISEDataSaver) Save( // We work out the default file name when copying output images now... because if there isn't one, we may pick one during that process. defaultContextImage, err := copyImagesToOutput(contextImageSrcPath, []string{data.DatasetID}, data.DatasetID, outputImagesPath, data, db, jobLog) + if err != nil { + return err + } + exp.MainContextImage = defaultContextImage // Set any matched aligned images - this happens after copyImagesToOutput because file names may be modified by it depending on formats @@ -359,7 +363,7 @@ func (s *PIXLISEDataSaver) Save( summaryData := makeSummaryFileContent(&exp, data.DatasetID, data.Instrument, data.Meta, int(fi.Size()), creationUnixTimeSec, data.CreatorUserId) - jobLog.Infof("Writing summary to DB...") + jobLog.Infof("Writing summary to DB for %v...", summaryData.Id) coll = db.Collection(dbCollections.ScansName) opt := options.Update().SetUpsert(true) @@ -374,7 +378,7 @@ func (s *PIXLISEDataSaver) Save( } // Set ownership - ownerItem, err := wsHelpers.MakeOwnerForWrite(summaryData.Id, protos.ObjectType_OT_SCAN, summaryData.CreatorUserId, creationUnixTimeSec) + ownerItem := wsHelpers.MakeOwnerForWrite(summaryData.Id, protos.ObjectType_OT_SCAN, summaryData.CreatorUserId, creationUnixTimeSec) ownerItem.Viewers = autoShare.Viewers ownerItem.Editors = autoShare.Editors @@ -382,6 +386,7 @@ func (s *PIXLISEDataSaver) Save( coll = db.Collection(dbCollections.OwnershipName) opt = options.Update().SetUpsert(true) + jobLog.Infof("Writing ownership to DB for scan %v...", ownerItem.Id) result, err = coll.UpdateOne(context.TODO(), bson.D{{Key: "_id", Value: ownerItem.Id}}, bson.D{{Key: "$set", Value: ownerItem}}, opt) if err != nil { jobLog.Errorf("Failed to write ownership item to DB: %v", err) @@ -408,10 +413,13 @@ func insertDefaultImage(db *mongo.Database, scanId string, defaultImage string, coll := db.Collection(dbCollections.ScanDefaultImagesName) opt := options.InsertOne() - defaultImageResult, err := coll.InsertOne(context.TODO(), &protos.ScanImageDefaultDB{ScanId: scanId, DefaultImageFileName: path.Join(scanId, defaultImage)}, opt) + imgPath := path.Join(scanId, defaultImage) + jobLog.Infof("Writing default image %v to DB for scan %v...", imgPath, scanId) + defaultImageResult, err := coll.InsertOne(context.TODO(), &protos.ScanImageDefaultDB{ScanId: scanId, DefaultImageFileName: imgPath}, opt) if err != nil { if mongo.IsDuplicateKeyError(err) { // Don't overwrite, so we're OK with this + jobLog.Infof("Default image for scan %v already exists, not overwriting existing one in case of user edit.", scanId) return nil } return err diff --git a/api/quantification/create.go b/api/quantification/create.go index f40dc4a3..2e36f6e3 100644 --- a/api/quantification/create.go +++ b/api/quantification/create.go @@ -382,12 +382,7 @@ func (r *quantNodeRunner) triggerPiquantNodes(wg *sync.WaitGroup) { }, } - ownerItem, err := wsHelpers.MakeOwnerForWrite(r.jobId, protos.ObjectType_OT_QUANTIFICATION, r.quantStartSettings.RequestorUserId, now) - if err != nil { - msg := fmt.Sprintf("Failed to create ownership info for quant job %v. Error was: %v", r.jobId, err) - r.completeJobState(false, msg, quantOutPath, piquantLogList) - return - } + ownerItem := wsHelpers.MakeOwnerForWrite(r.jobId, protos.ObjectType_OT_QUANTIFICATION, r.quantStartSettings.RequestorUserId, now) err = writeQuantAndOwnershipToDB(summary, ownerItem, svcs.MongoDB) if err != nil { diff --git a/api/quantification/importCSV.go b/api/quantification/importCSV.go index bc86cb84..c9278741 100644 --- a/api/quantification/importCSV.go +++ b/api/quantification/importCSV.go @@ -66,10 +66,7 @@ func ImportQuantCSV( } // Finally, write the summary data to DB along with ownership entry - ownerItem, err := wsHelpers.MakeOwnerForWrite(quantId, protos.ObjectType_OT_QUANTIFICATION, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return quantId, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(quantId, protos.ObjectType_OT_QUANTIFICATION, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) summary := protos.QuantificationSummary{ Id: quantId, diff --git a/api/ws/handlers/element-set.go b/api/ws/handlers/element-set.go index c79a7c70..b8aa244d 100644 --- a/api/ws/handlers/element-set.go +++ b/api/ws/handlers/element-set.go @@ -101,10 +101,8 @@ func createElementSet(elementSet *protos.ElementSet, hctx wsHelpers.HandlerConte elementSet.Id = id // We need to create an ownership item along with it - ownerItem, err := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_ELEMENT_SET, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_ELEMENT_SET, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) + elementSet.ModifiedUnixSec = ownerItem.CreatedUnixSec wc := writeconcern.New(writeconcern.WMajority()) diff --git a/api/ws/handlers/expression-group.go b/api/ws/handlers/expression-group.go index 88813af6..1c188dc3 100644 --- a/api/ws/handlers/expression-group.go +++ b/api/ws/handlers/expression-group.go @@ -106,10 +106,7 @@ func createExpressionGroup(egroup *protos.ExpressionGroup, hctx wsHelpers.Handle egroup.Id = id // We need to create an ownership item along with it - ownerItem, err := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_EXPRESSION_GROUP, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_EXPRESSION_GROUP, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) egroup.ModifiedUnixSec = ownerItem.CreatedUnixSec diff --git a/api/ws/handlers/expression.go b/api/ws/handlers/expression.go index 53a0b4ee..2464dcd3 100644 --- a/api/ws/handlers/expression.go +++ b/api/ws/handlers/expression.go @@ -110,10 +110,7 @@ func createExpression(expr *protos.DataExpression, hctx wsHelpers.HandlerContext expr.Id = id // We need to create an ownership item along with it - ownerItem, err := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_EXPRESSION, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_EXPRESSION, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) expr.ModifiedUnixSec = ownerItem.CreatedUnixSec diff --git a/api/ws/handlers/module.go b/api/ws/handlers/module.go index c06a3a19..87346ffb 100644 --- a/api/ws/handlers/module.go +++ b/api/ws/handlers/module.go @@ -262,10 +262,7 @@ func createModule(name string, comments string, intialSourceCode string, tags [] } // We need to create an ownership item along with it - ownerItem, err := wsHelpers.MakeOwnerForWrite(modId, protos.ObjectType_OT_DATA_MODULE, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(modId, protos.ObjectType_OT_DATA_MODULE, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) module.ModifiedUnixSec = ownerItem.CreatedUnixSec diff --git a/api/ws/handlers/roi.go b/api/ws/handlers/roi.go index 8f83d88d..337decd3 100644 --- a/api/ws/handlers/roi.go +++ b/api/ws/handlers/roi.go @@ -180,10 +180,7 @@ func createROI(roi *protos.ROIItem, hctx wsHelpers.HandlerContext, needMistEntry roi.Id = id // We need to create an ownership item along with it - ownerItem, err := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_ROI, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + ownerItem := wsHelpers.MakeOwnerForWrite(id, protos.ObjectType_OT_ROI, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) roi.ModifiedUnixSec = ownerItem.CreatedUnixSec diff --git a/api/ws/handlers/screen-configuration.go b/api/ws/handlers/screen-configuration.go index 7759a090..96e9625e 100644 --- a/api/ws/handlers/screen-configuration.go +++ b/api/ws/handlers/screen-configuration.go @@ -156,10 +156,7 @@ func writeScreenConfiguration(screenConfig *protos.ScreenConfiguration, hctx wsH } // We need to create an ownership item along with it - owner, err = wsHelpers.MakeOwnerForWrite(screenConfig.Id, protos.ObjectType_OT_SCREEN_CONFIG, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) - if err != nil { - return nil, err - } + owner = wsHelpers.MakeOwnerForWrite(screenConfig.Id, protos.ObjectType_OT_SCREEN_CONFIG, hctx.SessUser.User.Id, hctx.Svcs.TimeStamper.GetTimeNowSec()) screenConfig.ModifiedUnixSec = owner.CreatedUnixSec configuration = screenConfig diff --git a/api/ws/wsHelpers/ownership.go b/api/ws/wsHelpers/ownership.go index 7f220c62..15c2bbdd 100644 --- a/api/ws/wsHelpers/ownership.go +++ b/api/ws/wsHelpers/ownership.go @@ -15,7 +15,7 @@ import ( "go.mongodb.org/mongo-driver/mongo/options" ) -func MakeOwnerForWrite(objectId string, objectType protos.ObjectType, creatorUserId string, createTimeUnixSec int64) (*protos.OwnershipItem, error) { +func MakeOwnerForWrite(objectId string, objectType protos.ObjectType, creatorUserId string, createTimeUnixSec int64) *protos.OwnershipItem { ownerItem := &protos.OwnershipItem{ Id: objectId, ObjectType: objectType, @@ -30,7 +30,7 @@ func MakeOwnerForWrite(objectId string, objectType protos.ObjectType, creatorUse } } - return ownerItem, nil + return ownerItem } // Checks object access - if requireEdit is true, it checks for edit access diff --git a/core/beamLocation/experimentFileToDB.go b/core/beamLocation/experimentFileToDB.go index 27b3b22d..7c49fff9 100644 --- a/core/beamLocation/experimentFileToDB.go +++ b/core/beamLocation/experimentFileToDB.go @@ -47,6 +47,8 @@ func ImportBeamLocationToDB(imgName string, instrument protos.ScanInstrument, fo }) opt := options.Replace().SetUpsert(true) + logger.Infof("Writing beam location to DB for image: %v, and scan: %v, instrument: %v, beamVersion: %v", imgName, forScanId, instrument, beamVersion) + result, err := imagesColl.ReplaceOne(context.TODO(), bson.M{"_id": imgName}, beams, opt) if err != nil { return err From 09babd7dbaa36d0a31259a1735b8ec7c7450904d Mon Sep 17 00:00:00 2001 From: Peter Nemere Date: Wed, 27 Mar 2024 10:57:13 +1000 Subject: [PATCH 2/2] Fixing all situations where importer needs width/height of image being read, and that image is a RGBU TIF, which failed to be read by Go tif lib. We now detect it's an RGBU TIF and use a hard-coded width/height --- api/coreg/import.go | 17 ++++++++--- .../internal/converterSelector/selector.go | 4 +-- api/dataimport/internal/output/output.go | 25 ++++++++++++++--- api/ws/handlers/image.go | 8 +++--- core/utils/images.go | 28 +++++++++++-------- 5 files changed, 56 insertions(+), 26 deletions(-) diff --git a/api/coreg/import.go b/api/coreg/import.go index 52dab56a..699da0b7 100644 --- a/api/coreg/import.go +++ b/api/coreg/import.go @@ -257,10 +257,15 @@ func importNewImage(jobId string, imageUrl string, baseRTT string, marsViewerExp return "", nil, nil, err } + imgWidth := uint32(0) + imgHeight := uint32(0) + var img image.Image if imageExt == ".IMG" { w, h, d, err := imgFormat.ReadIMGFile(imgData) + imgWidth = uint32(w) + imgHeight = uint32(h) if err != nil { return "", nil, nil, fmt.Errorf("Failed to read IMG file: %v. Error: %v", imageUrl, err) } @@ -280,7 +285,7 @@ func importNewImage(jobId string, imageUrl string, baseRTT string, marsViewerExp imgData = buf.Bytes() } else { - img, _, err = image.Decode(bytes.NewReader(imgData)) + imgWidth, imgHeight, err = utils.ReadImageDimensions(path.Base(imgPath), imgData) if err != nil { return "", nil, nil, fmt.Errorf("Failed to read image file: %v. Error: %v", imageUrl, err) } @@ -296,7 +301,9 @@ func importNewImage(jobId string, imageUrl string, baseRTT string, marsViewerExp "", //baseRTT, "", nil, - img) + imgWidth, + imgHeight, + ) ctx := context.TODO() coll := hctx.Svcs.MongoDB.Collection(dbCollections.ImagesName) @@ -523,7 +530,7 @@ func importWarpedImage(warpedImageUrl string, rttWarpedTo string, baseImage stri return err } - img, _, err := image.Decode(bytes.NewReader(imgData)) + imgWidth, imgHeight, err := utils.ReadImageDimensions(path.Base(warpedSrcPath), imgData) if err != nil { return err } @@ -545,7 +552,9 @@ func importWarpedImage(warpedImageUrl string, rttWarpedTo string, baseImage stri rttWarpedTo, "", matchInfo, - img) + imgWidth, + imgHeight, + ) ctx := context.TODO() coll := hctx.Svcs.MongoDB.Collection(dbCollections.ImagesName) diff --git a/api/dataimport/internal/converterSelector/selector.go b/api/dataimport/internal/converterSelector/selector.go index c15c6356..ab239afb 100644 --- a/api/dataimport/internal/converterSelector/selector.go +++ b/api/dataimport/internal/converterSelector/selector.go @@ -92,9 +92,9 @@ func SelectDataConverter(localFS fileaccess.FileAccess, remoteFS fileaccess.File // Log the paths to help us diagnose issues... // Print it in one log message - logMsg := "SelectDataConverter path listing:" + logMsg := "SelectDataConverter path listing:\n" for c, item := range items { - logMsg += fmt.Sprintf("\n %v. %v\n", c+1, item) + logMsg += fmt.Sprintf(" %v. %v\n", c+1, item) } log.Infof(logMsg) diff --git a/api/dataimport/internal/output/output.go b/api/dataimport/internal/output/output.go index 909aa498..e67d6e2a 100644 --- a/api/dataimport/internal/output/output.go +++ b/api/dataimport/internal/output/output.go @@ -319,7 +319,7 @@ func (s *PIXLISEDataSaver) Save( // We work out the default file name when copying output images now... because if there isn't one, we may pick one during that process. defaultContextImage, err := copyImagesToOutput(contextImageSrcPath, []string{data.DatasetID}, data.DatasetID, outputImagesPath, data, db, jobLog) if err != nil { - return err + return fmt.Errorf("Error copying images: %v", err) } exp.MainContextImage = defaultContextImage @@ -664,19 +664,36 @@ func insertImageDBEntryForImage( matchInfo *protos.ImageMatchTransform, jobLog logger.ILogger) error { // Read the image - we used to only copy files around but here we need to open it for meta data - imgFile, err := utils.ReadImageFile(imagePath) + imgbytes, err := os.ReadFile(imagePath) if err != nil { return err } - //defer imgFile.Close() + imgWidth, imgHeight, err := utils.ReadImageDimensions(imagePath, imgbytes) + if err != nil { + return err + } stats, err := os.Stat(imagePath) + if err != nil { + return err + } saveName := filepath.Base(imagePath) savePath := path.Join(originScanId, saveName) - img := utils.MakeScanImage(savePath, uint32(stats.Size()), source, purpose, associatedScanIds, originScanId, originImageURL, matchInfo, imgFile) + img := utils.MakeScanImage( + savePath, + uint32(stats.Size()), + source, + purpose, + associatedScanIds, + originScanId, + originImageURL, + matchInfo, + imgWidth, + imgHeight, + ) return insertImageDBEntry(db, img, jobLog) } diff --git a/api/ws/handlers/image.go b/api/ws/handlers/image.go index 4ab7adc7..b3bdafa4 100644 --- a/api/ws/handlers/image.go +++ b/api/ws/handlers/image.go @@ -1,11 +1,9 @@ package wsHandler import ( - "bytes" "context" "errors" "fmt" - "image" "net/http" "path" "strings" @@ -338,7 +336,7 @@ func HandleImageUploadReq(req *protos.ImageUploadReq, hctx wsHelpers.HandlerCont db := hctx.Svcs.MongoDB // Save image meta in collection - img, _, err := image.Decode(bytes.NewReader(req.ImageData)) + imgWidth, imgHeight, err := utils.ReadImageDimensions(req.Name, req.ImageData) if err != nil { return nil, err } @@ -364,7 +362,9 @@ func HandleImageUploadReq(req *protos.ImageUploadReq, hctx wsHelpers.HandlerCont req.OriginScanId, "", req.GetBeamImageRef(), - img) + imgWidth, + imgHeight, + ) coll = db.Collection(dbCollections.ImagesName) diff --git a/core/utils/images.go b/core/utils/images.go index 21f60c33..35812483 100644 --- a/core/utils/images.go +++ b/core/utils/images.go @@ -28,18 +28,21 @@ import ( protos "github.com/pixlise/core/v4/generated-protos" ) -func ReadImageFile(path string) (image.Image, error) { - // Load the full context image from test data - imgbytes, err := os.ReadFile(path) +// Returns width, height and error +func ReadImageDimensions(imageName string, imgBytes []byte) (uint32, uint32, error) { + // Try to read the image + img, _, err := image.Decode(bytes.NewReader(imgBytes)) if err != nil { - return nil, err - } + // Failed, maybe it's a RGBU TIF image + upperName := strings.ToUpper(imageName) + if strings.Contains(err.Error(), "sample format") && (strings.Contains(upperName, "VIS_") || strings.Contains(upperName, "MSA_")) && strings.HasSuffix(upperName, ".TIF") { + // We can't read these tif files, but it's an RGBU image, and they have a known resolution - the same as our MCC images + return 752, 580, nil + } - img, _, err := image.Decode(bytes.NewReader(imgbytes)) - if err != nil { - return nil, err + return 0, 0, err } - return img, nil + return uint32(img.Bounds().Dx()), uint32(img.Bounds().Dy()), nil } func ImagesEqual(aPath, bPath string) error { @@ -111,13 +114,14 @@ func MakeScanImage( originScanId string, originImageURL string, matchInfo *protos.ImageMatchTransform, - img image.Image) *protos.ScanImage { + width uint32, + height uint32) *protos.ScanImage { result := &protos.ScanImage{ ImagePath: imgPath, Source: source, - Width: uint32(img.Bounds().Dx()), - Height: uint32(img.Bounds().Dy()), + Width: width, + Height: height, FileSize: fileSize, Purpose: purpose,