Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-9132] Add (Get)Image to the camera interface #4487
base: main
Are you sure you want to change the base?
[RSDK-9132] Add (Get)Image to the camera interface #4487
Changes from 3 commits
a643d22
41cb592
f6e3d69
d6439dd
6417a56
59c36ec
16079fa
9084264
c44afa2
11b1d7d
438d550
0d8081b
824c30f
e744b68
fd50881
e570393
6646d78
612e91c
9029a05
6ec0041
ef1bd0e
d01159a
44611d5
146345f
c698e16
9da582f
1b51109
eaf28d7
4fe7e36
b4e1960
fc6665d
8afb714
375a35f
5bf744c
2a1cd8b
1233170
95f3f42
b208d2c
0b85975
c44454b
d848d20
09e295c
3edf860
b7c4635
63b7681
c91b68d
d63980e
6ffbae2
65b5bad
9a9612f
ab7e736
8244dfd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
camera.ReadImage still exists & should still behave the same way id did right?
If so, can we please not remove the
camera.ReadImage
test & just add thecamera.ImageFromVideoSource
tests?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.
It is being tested all throughout the transform camera.
Also - these two tests aren't doing much, they're testing projector/ no projector functionality, which was removed, and they can eventually be removed.
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.
Added back said tests that make sense: 8244dfd
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.
Could we
Read
and handle release so we do not need to include release func in returns? (perhaps that would make sense in the videosourcewrapper layer).As mentioned, this would be a larger departure from Stream.Next which assuming will cause major problems.
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.
I'm gonna try to get rid of release in camera components and see what happens
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.
Looks like we just need to move the
rimage.EncodeImage
step one level in i.e.outBytes
should come out of theImage
call instead of being handled in the data collector or camera server/client. Currentlyrelease
is called in the collector/server/client, so we should just move it into theImage
implementation. Same logic, just handled more nested in our abstraction.I guess future module writers should get used to using
rimage
to encode their output i.e. read from the source bytes and output a newly constructed[]byte
result? Does that sound okay to everyone?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.
In the client, I think that makes sense to return
rimage
to match the python sdk functionality -- avoid unnecessary decodes if you just want bytes.Would this be similar to
viamimage
in the python sdk, i think that works pretty well.Looks like the server is already handling release and encoding for the caller.
Are you also suggesting removing encoding step in server GetImage?
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 is my WIP in
server.go
So I think yes, in the default case we don't encode anymore since the return type is now bytes
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.
Ok cool I like that.
I am little confused on why we still need the
ReadImager
path here. Shouldnt the camera interface now always haveImage
defined so we can just use that?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.
My thinking here is since
viamrtsp
uses theVideoReaderFunc
andRead
'srelease
functionality to keep track of pool frames in the frame allocation optimization flow, for theserver.go
that servesviamrtsp
, we need to be able to callrelease()
I think we could refactor
viamrtsp
though to just copy out the bytes on return early and use the new.Image
pathway... I'm down to removeReadImager
entirely as long as we make it a high priority to refactorviamrtsp
to use.Image
and[]byte
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.
I see, interesting. I think it makes sense to leave in for now for viamrtsp compatibility.
Yep the whole point to having a memory manager was the issue with passing in a pointer to the avframe in
VideoReaderFunc
return causing segfaults with the encoding in the server.Since we are removing encoding from the server and relying on the user to handle this, we should eventually be able to use Image pathway and simplify things quite a bit : )
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.
All our go cameras will need a refactor for the full fix of this interface, I do not anticipate that we will have the best version of this code and go modules after one pr. Eventually we will get rid of Stream and Next completely. The problem is those videosourcewrapper helpers are all over the place in go modules since they're exported convenience functions.
Always export code conscientiously. But we're breaking this interface anyway, so we'll deal with the following breaks.
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.
Okay 👍 will remove ReadImager in this PR
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.
given stream still exists & it's behavior hasn't changed, can we please just make these additive changes, rather than removing the Stream tests?
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.
Leaving this test deleted since we no longer have this helper function