Skip to content

Conversation

@eliteprox
Copy link
Collaborator

@eliteprox eliteprox commented Apr 14, 2025

This change makes use of the exported pipeline class from livepeer/comfystream#114

This change is dependent on livepeer/comfystream#176

@eliteprox eliteprox force-pushed the feat/comfystream-pipeline branch from fd14d5a to adff001 Compare April 14, 2025 21:32
@eliteprox eliteprox force-pushed the feat/comfystream-pipeline branch 2 times, most recently from 1356f0a to 75b2a2f Compare April 15, 2025 23:48
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Neat! Requesting changes though because there's a regression on the handling of the request_id. It's basically rolling back #525

Comment on lines -84 to -86
out = await self.video_incoming_frames.get()
while out.frame.side_data.skipped:
out = await self.video_incoming_frames.get()
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this logic anymore?

Copy link
Collaborator Author

@eliteprox eliteprox May 5, 2025

Choose a reason for hiding this comment

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

Don't we need this logic anymore?

The frame skipping logic is in comfystream here https://github.com/livepeer/comfystream/blob/e9b87bc7bc0adb32c5731f70b1e11e44c1f2e3c1/src/comfystream/pipeline.py#L169-L170, so this method already implements the logic internally


@abstractmethod
async def put_video_frame(self, frame: VideoFrame, request_id: str):
async def put_video_frame(self, frame: VideoFrame):
Copy link
Member

Choose a reason for hiding this comment

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

This is a regression. The request_id is defined on the put not on the get.

Copy link
Collaborator Author

@eliteprox eliteprox May 5, 2025

Choose a reason for hiding this comment

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

Thanks for pointing out the solution here, I have opened a PR to comfystream that will support passing an optional request_id into the frame side_data. Tested this solution successfully

@eliteprox eliteprox force-pushed the feat/comfystream-pipeline branch from 75b2a2f to 10e9ffb Compare April 29, 2025 19:54
@eliteprox eliteprox marked this pull request as ready for review May 5, 2025 17:20
@eliteprox eliteprox force-pushed the feat/comfystream-pipeline branch 3 times, most recently from bb4d668 to e9691da Compare May 14, 2025 20:56
@eliteprox eliteprox force-pushed the feat/comfystream-pipeline branch from e9691da to 131c2ce Compare May 14, 2025 21:39
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