-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RSDK-9660] - fix-camera-image-returning-no-mime-type #4674
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,11 +196,14 @@ func (c *client) Image(ctx context.Context, mimeType string, extra map[string]in | |
|
||
if expectedType != "" && resp.MimeType != expectedType { | ||
c.logger.CDebugw(ctx, "got different MIME type than what was asked for", "sent", expectedType, "received", resp.MimeType) | ||
} else { | ||
resp.MimeType = mimeType | ||
if resp.MimeType == "" { | ||
// if the user expected a mime_type and the successful response didn't have a mime type, assume the | ||
// response's mime_type was what the user requested | ||
resp.MimeType = mimeType | ||
} | ||
} | ||
|
||
return resp.Image, ImageMetadata{MimeType: utils.WithLazyMIMEType(resp.MimeType)}, nil | ||
return resp.Image, ImageMetadata{MimeType: resp.MimeType}, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm moving this |
||
} | ||
|
||
func (c *client) Images(ctx context.Context) ([]NamedImage, resource.ResponseMetadata, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"context" | ||
"image" | ||
"image/color" | ||
"image/jpeg" | ||
"image/png" | ||
"net" | ||
"testing" | ||
|
@@ -428,7 +429,14 @@ func TestClientLazyImage(t *testing.T) { | |
test.That(t, png.Encode(&imgBuf, img), test.ShouldBeNil) | ||
imgPng, err := png.Decode(bytes.NewReader(imgBuf.Bytes())) | ||
test.That(t, err, test.ShouldBeNil) | ||
var jpegBuf bytes.Buffer | ||
test.That(t, jpeg.Encode(&jpegBuf, img, &jpeg.Options{Quality: 100}), test.ShouldBeNil) | ||
imgJpeg, err := jpeg.Decode(bytes.NewBuffer(jpegBuf.Bytes())) | ||
test.That(t, err, test.ShouldBeNil) | ||
|
||
injectCamera.PropertiesFunc = func(ctx context.Context) (camera.Properties, error) { | ||
return camera.Properties{}, nil | ||
} | ||
injectCamera.ImageFunc = func(ctx context.Context, mimeType string, extra map[string]interface{}) ([]byte, camera.ImageMetadata, error) { | ||
if mimeType == "" { | ||
mimeType = rutils.MimeTypeRawRGBA | ||
|
@@ -470,7 +478,6 @@ func TestClientLazyImage(t *testing.T) { | |
frameLazy := frame.(*rimage.LazyEncodedImage) | ||
test.That(t, frameLazy.RawData(), test.ShouldResemble, imgBuf.Bytes()) | ||
|
||
ctx = context.Background() | ||
frame, err = camera.DecodeImageFromCamera(ctx, rutils.WithLazyMIMEType(rutils.MimeTypePNG), nil, camera1Client) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, frame, test.ShouldHaveSameTypeAs, &rimage.LazyEncodedImage{}) | ||
|
@@ -482,6 +489,24 @@ func TestClientLazyImage(t *testing.T) { | |
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, compVal, test.ShouldEqual, 0) // exact copy, no color conversion | ||
|
||
// when DecodeImageFromCamera is called without a mime type, defaults to JPEG | ||
var called bool | ||
injectCamera.ImageFunc = func(ctx context.Context, mimeType string, extra map[string]interface{}) ([]byte, camera.ImageMetadata, error) { | ||
called = true | ||
test.That(t, mimeType, test.ShouldResemble, rutils.WithLazyMIMEType(rutils.MimeTypeJPEG)) | ||
resBytes, err := rimage.EncodeImage(ctx, imgPng, mimeType) | ||
test.That(t, err, test.ShouldBeNil) | ||
return resBytes, camera.ImageMetadata{MimeType: mimeType}, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change to non lazy |
||
} | ||
frame, err = camera.DecodeImageFromCamera(ctx, "", nil, camera1Client) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, frame, test.ShouldHaveSameTypeAs, &rimage.LazyEncodedImage{}) | ||
frameLazy = frame.(*rimage.LazyEncodedImage) | ||
test.That(t, frameLazy.MIMEType(), test.ShouldEqual, rutils.MimeTypeJPEG) | ||
compVal, _, err = rimage.CompareImages(imgJpeg, frame) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, compVal, test.ShouldEqual, 0) // exact copy, no color conversion | ||
test.That(t, called, test.ShouldBeTrue) | ||
test.That(t, conn.Close(), test.ShouldBeNil) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,10 @@ import ( | |
"context" | ||
"image" | ||
"image/color" | ||
"net/http" | ||
"sync" | ||
|
||
"go.viam.com/rdk/logging" | ||
) | ||
|
||
// LazyEncodedImage defers the decoding of an image until necessary. | ||
|
@@ -29,6 +32,11 @@ type LazyEncodedImage struct { | |
// away with reading all metadata from the header of the image bytes. | ||
// NOTE: Usage of an image that would fail to decode causes a lazy panic. | ||
func NewLazyEncodedImage(imgBytes []byte, mimeType string) image.Image { | ||
if mimeType == "" { | ||
logging.Global().Warn("NewLazyEncodedImage called without a mime_type. " + | ||
"Sniffing bytes to detect mime_type. Specify mime_type to reduce CPU utilization") | ||
mimeType = http.DetectContentType(imgBytes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will attempt to detect the mime type of the image bytes if mimeType is empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a cool function from that library - nice to know it's in there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems this method can return 3 mime types that would be interesting to us:
It might be worth logging if this code path is getting application/octet-stream There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great suggestion, done |
||
} | ||
return &LazyEncodedImage{ | ||
imgBytes: imgBytes, | ||
mimeType: mimeType, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes it so that we only override the camera.GetImageResponse.MimeType if it is empty string and the caller specified an expected mime type.