Skip to content
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-9920] - add lazy_decode #85

Merged

Conversation

nicksanford
Copy link
Collaborator

@nicksanford nicksanford commented Feb 5, 2025

Changes:

https://viam.atlassian.net/browse/RSDK-9920

  1. Add new config parameter: lazy_decode: bool, default: false
  2. Only applies if codec is h264. If set and codec is anything else, log a warning at startup.
  3. If set to true, accumulate the NALUs (h264 messages) since the last IDR. Only compute an image from those NALUs when camera.Image is called.

Performance Impact:

Hardware: Pi 5:

Resolution: 2304*1296 (larger than 1080p)
Frame Rate (FPS): 25
Max Bitrate (Kbps): 4096
I-frame Interval: 2x (about every 2 seconds)

Baseline lazy_decode: false or unset:

  1. no callers: htop reports ~35% cpu
  2. with gostream calling camera.Image as fast as it can: htop reports ~50% cpu, each request takes ~35 ms

lazy_decode: true

  1. no callers: htop reports ~2% cpu
  2. with gostream calling camera.Image as fast as it can: htop reports ~50% cpu, each request takes 100-500 ms, depending on how frequently camera.Image is called. When called more frequently, fewer NALUs need to be processed and the image can be computed and returned faster.

Here is a video of the experience of each config option.

  1. first is with rtp_passthrough: true (best and the default)
  2. second is with rtp_passthrough: false, lazy_decode: false
  3. third is with rtp_passthrough: false, lazy_decode: true
Kapture.2025-02-05.at.14.46.54.mp4

@nicksanford nicksanford marked this pull request as ready for review February 5, 2025 20:03

closeMu sync.RWMutex
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to add this as now both Image and OnPacket's callback (which are called from 2 different goroutines) can try to interact with the pool and the decoder. As a result, I made it so that Image can execute concurrently with other Image calls but can't run concurrently with Close and such that it will early exist if close has already been called.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like it should be implemented with our StoppableWorker structure to handle this exact case, can you make a TODO and a ticket to look into this.

Are you seeing cases where Close is not stopping the Image call? If so there's probably a bug or something being badly freed/uncancelled in this code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a similar shape of problem to what we use StoppableWorker to solve, however I don't think StoppableWorker is the right solution to this problem as the goroutine calling Image is spawned by the camera/server.go hosted in the module, not by our code.

I think the approach I used here is reasonable. Close won't do it's thing until all in progress Image calls complete and once Close is called, Image won't execute.


closeMu sync.RWMutex
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like it should be implemented with our StoppableWorker structure to handle this exact case, can you make a TODO and a ticket to look into this.

Are you seeing cases where Close is not stopping the Image call? If so there's probably a bug or something being badly freed/uncancelled in this code.

@@ -250,6 +258,7 @@ func (rc *rtspCamera) closeConnection() {
rc.client.Close()
rc.client = nil
}
rc.resetAU(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] Why reset to nil rather than an empty array of bytes like in the consumer helper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. I'll change consumeAU to set rc.au to nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

nicksanford and others added 2 commits February 5, 2025 17:37
@nicksanford nicksanford merged commit 41ab2fc into viam-modules:main Feb 6, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants