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

Add image attach feature to chat component #932

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

s-hamdananwar
Copy link

@s-hamdananwar s-hamdananwar commented Jul 27, 2024

I have been wanting to implement an image upload feature for meet.livekit.io for some time now, and I finally got some time to hack on it! I also think this could be a good example that demonstrates the different use cases of Data Messages.

Currently, this feature would only support image uploading, but I believe down the road it could be expanded to multiple images and/or different file types.

Image Message Sending Architecture

  1. User uploads image
  2. Image data is divided into packets of 50k byte (LK size limit for data messages is ~65k I believe?)
  3. Each packet is sent as a LiveKit data message, with each message holding image packet properties like message id, packet index, total packet count and image data

Image Message Receiving Architecture

  1. Browser receives LiveKit data message which holds image properties
  2. Browser waits until all packets are arrived, ie. does not send individual packets to the user
  3. Once final packet is arrived, it creates an image blob concatenating all the image data
  4. The image blob is attached to a ChatMessage object and is send to the user
  5. If a message has an image attached to it, the component would display the image

Another way I considered implementing this feature was to create a new Observable object just for images, but I wasn't sure if that would be too much of an architectural change. I would also have to have two observables handling all incoming messages which maybe is not desired?

Test Findings
I tested images with size up to 5MB and although they worked most of the time, I did find some rare instances where packet loss occurs. Images above 8 MB failed due to packet loss almost all the time. I am not sure if this is expected, even when the message mode is set to be reliable. I would love to hear if anyone have any insights on this! I set the maximum file size threshold at 3 MB (for now?) as in my testing, files lesser than 3 MB always went through fine and quick. I also tested concurrent file sending between two users in a room, and that seem to work as expected as well. I would like to probably test this feature with multiple users sending images concurrently to the room.

I would love to hear the team's and the community's thoughts, suggestions, concerns, bugs(if any) and test findings, and would love to make according changes! A sample demo of the feature can be found in this Slack message!

Copy link

changeset-bot bot commented Jul 27, 2024

⚠️ No Changeset found

Latest commit: 196ead6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@lukasIO
Copy link
Contributor

lukasIO commented Aug 6, 2024

hey @s-hamdananwar ,

thank you for your contribution, this is an impressive effort!

We'll have to decide internally first whether we want to go down the route of distributing chat message assets via datachannel or maybe in some other form.
If we decide that DataChannel usage is the way to go, we'll first come back to this PR.

@s-hamdananwar
Copy link
Author

Hi @lukasIO, thanks for your response!
I completely understand, and I look forward to you and your team's decision.

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