Skip to content

Conversation

@ad-astra-video
Copy link
Contributor

Quick fix for when a publisher is not started because video egress is turned off to not add to output queue.

Output queue has a max of 200 but at larger frame size memory usage expands quite a bit.

Copy link
Collaborator

@eliteprox eliteprox left a comment

Choose a reason for hiding this comment

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

@ad-astra-video Thanks for resolving this critical bug which was likely introduced in #36

I noticed the tests are hanging on your change. Can you test if the change in #93 solves the issue the same?

if self.publish_task is None:
logger.debug("Skipping enqueuing frame to publish queue (no publisher configured)")
continue
await asyncio.to_thread(enqueue_frame, frame)

This will make it easier to reference publish_task and ensure the frames make it back to the protocol for keeping egress fps accurate

@ad-astra-video
Copy link
Contributor Author

The fix in #93 seems to work as well so I have used that fix here now in the protocol rather than the client.

Do you think we should record egress metrics if not sending or put the continue before that?

I think there are two sides of it:

  • may be confusing if we record egress metrics if nothing actually is being returned
  • other side of the argument is having egress metrics would continue to provide a throughput measurement of the pipeline to enable comparing ingress/egress fps metrics

@eliteprox
Copy link
Collaborator

You make a good point, I could see it both ways.

I think we should not record egress FPS when there is no media publisher. It might be confusing to see egress_fps in stream metrics with no media publishing configured.

For a pipeline which has no video/audio output and only produces text, perhaps we could later add a new metric like "data messages per second"

@eliteprox
Copy link
Collaborator

I'm also fine with keeping the egress metric, it is good for visibility and analyzing frame processor performance

Copy link
Collaborator

@eliteprox eliteprox left a comment

Choose a reason for hiding this comment

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

LGTM

@ad-astra-video ad-astra-video merged commit d7e8852 into main Dec 24, 2025
4 checks passed
@ad-astra-video ad-astra-video deleted the fix-holding-frames branch December 24, 2025 16:52
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.

3 participants