RSDK-8663: Optimize away gostream concurrency complexity for camera clients.

[dgottlieb]

No description provided.

[hexbabe]

@dgottlieb looks like CI is failing FYI

[dgottlieb]

Thanks -- did you otherwise review this?

[hexbabe]

Did we pinpoint the reason why two GetImage calls are sometimes made? Is it possible to prevent that instead

[hexbabe]

Curious syntax

Wondering why you prefer this?

[dgottlieb]

How this code got this way is a bit of a long story -- it wasn't due to preference. I was trying to use the "test.ShouldImplement" assertion, that I think requires reflection and additional error handling. But ultimately failed at that. So the structure of this if-statement was already there and I just had to use a type-assertion instead and do nothing in the "then" clause.

That said, I do think writing conditionals in the affirmative (i.e: avoid double negatives) reads better. And in this case, a real writer is going to write the if-statement that way -- just with a non-empty "then".

But normally I wouldn't take that to the "extreme" of having an empty "then" clause. I just left it this way because I was fed up with writing an assertion.

[hexbabe]

Haha the test lib use of reflection can be super funky

Got it

[hexbabe]

I see since the client Read func is guaranteed to only call GetImage once in its body

[dgottlieb]

Yes! That's exactly how this patch works. Instead of spinning up:

• stream = camera.Stream()
• stream.Next()
• stream.Close

We first ask "Hey camera, do you have a "Read" method?" Great -- we'll just call that one time and avoid this stream non-sense.

[dgottlieb]

Did we pinpoint the reason why two GetImage calls are sometimes made?

Yes. The ticket/comments try to explain what happens. But it is hard to follow how that relates to the exact code because of the gostream layers involved.

Is it possible to prevent that instead

Given this patch does prevent that with camera clients, I assume you mean in the more general case? Yes and no. Right now, the API exposed by all underlying gostream cameras are:

• Images
• Stream

Beyond those two, cameras may not have any* other methods[1]. So for those that do, we can try and guess/use them. But even for the Read method that camera clients have, I'm not seeing many other cameras implement that. For example, the h264 camera becomes a "sourceBasedCamera" which only has the Images/Stream APIs + the rtp passthrough apis.

Point being, avoiding "double get image" from a bigger set of cameras requires more typing across the codebase. And the camera client was 90% of our problem today.

[1] As far as I can tell -- it's not correct to blindly use [Get]Images(req)[0] as a substitute for [Get]Image, e.g:

func (s *Server) GetImage(req *ImageRequest) {
  return camera.Images(...)[0]
}

[hexbabe]

Thanks for that info

From the writeup on the ticket:

The issue is, the background thread in camera client's Stream would have already called the c.Read() again, so the second request will be invoked and almost immediately cancelled.

I guess my question was more pointed towards this. There isn't a clear path forward preventing the double call originating from this background thread is there?

[dgottlieb]

I guess my question was more pointed towards this. There isn't a clear path forward preventing the double call originating from this background thread is there?

I explored an avenue where ReadImage/ReadMedia would do:

• stream = camera.Stream()
• image = stream.NextAndClose()

Which I think is what you're asking? Where we can "atomically" get a single image and communicate to the "producer" to not (eagerly) produce the next image.

I didn't find that to be possible without significantly mucking with the producerConsumer code. Which would then require a much more thorough proof of correctness/review.

[hexbabe]

I guess my question was more pointed towards this. There isn't a clear path forward preventing the double call originating from this background thread is there?

I explored an avenue where ReadImage/ReadMedia would do:

• stream = camera.Stream()
• image = stream.NextAndClose()

Which I think is what you're asking? Where we can "atomically" get a single image and communicate to the "producer" to not (eagerly) produce the next image.

I didn't find that to be possible without significantly mucking with the producerConsumer code. Which would then require a much more thorough proof of correctness/review.

Makes sense. That sounds very involved