search

algora-io / tv

Open source Twitch for developers
https://algora.tv
GNU Affero General Public License v3.0
867 stars 48 forks source link

Add Admin.merge_streams function #76

Closed Lirianer closed 2 days ago

Lirianer commented 2 weeks ago

Video

https://github.com/user-attachments/assets/79c3ad46-5229-468d-91d9-6a6c6051d2a9

Additions

Risk

I am almost certain that if we try to call Algora.Clipper.create_clip/3 with a merged vod it will break due to the Algora.Clipper.to_absolute/3 constructing the wrong uri as the merged vod segment uris are already absolute

Questions

Notes

I've copied the bucket and to_absolute functions from the Algora.Clipper module, maybe they could be reused from the module or moved to another one

/claim #71

zcesur commented 2 weeks ago

Awesome, will take a look at it tmr. Thanks for sharing a demo!

Re: Absolute URLs we should probably just use maybe_to_absolute everywhere instead of to_absolute to be safe. Feel free to move common functions into Library or anywhere you see fit

Which browser/device are you using btw? It'd be nice if we can test this on iOS, macOS and Safari in case discontinuity tag causes any trouble there

Lirianer commented 2 weeks ago

The demo was on Firefox / macOS. Just tested it on Safari and it works, I don't have an iOS device to test it though.

I'll take a look into using maybe_to_absolute and rearranging the functions later!

zcesur commented 2 weeks ago

Works great overall! No worries about iOS, I'll find someone else to test it

There's a slight glitch at the discontinuity point (0:46) where we only see the last frame of the first segment throughout its duration :thinking:

https://github.com/user-attachments/assets/7500f533-c374-4098-a6a7-ca472996fd60

Are we recalculating top-level playlist fields? These would be

If not let's do that which might fix the above issue. We should be able to derive these from existing playlists instead of recalculating from scratch. Check this out to see how they are defined

Re: properties of the generated video let's just use the properties of the first video (except for unique fields like uuid, url etc.)

Re: vallidation, we should ensure the resolution and the codec are the same across all playlists. Thanks for bringing this up!

Lastly, can we add a field like merged_into_id so that we can hide these videos from the user? Unlisted videos still show in user's channel (which is a separate issue) and studio which might be confusing

zcesur commented 2 weeks ago

Actually scratch the last bit about merged_into_id. I just added soft deletion for videos 71d883f so you can just use that

Lirianer commented 1 week ago

I've done the changes you said:

The target duration was already being changed to be the max one.

*About this first point I wasn't able to reproduce the glitch in your video so if this doesnt fix it maybe we should use the max avg_bandwidth as the avg_bandwidth instead of the avg of all the playlists as the new avg being calculated can be less than the max of one of the playlists

zcesur commented 1 week ago

Re: the glitch maybe the issue is browser/device specific then. Can you add the record below into your db and tell me how the clock at top right in the video ticks? Expected is

12:55:05 -> 06 -> ... -> 21 -> 22 -> [discontinuity] -> 25 -> 26 -> 27 -> 28 -> 29 -> 30

but in my case (Linux & Chrome) it is

12:55:05 -> 06 -> ... -> 21 -> 22 -> [discontinuity] -> 29 -> 29 -> 29 -> 29 -> 29 -> 30

Repro
```sql INSERT INTO videos ( duration, title, type, url, url_root, uuid, visibility, user_id, inserted_at, updated_at, thumbnail_url, format, filename, remote_path, og_image_url ) VALUES ( 35, 'Merged VOD', 1, 'https://fly.storage.tigris.dev/mediadev/597cb428-c830-45e5-834f-6bfb7b0e035a/index.m3u8', 'https://fly.storage.tigris.dev/mediadev/597cb428-c830-45e5-834f-6bfb7b0e035a', '597cb428-c830-45e5-834f-6bfb7b0e035a', 1, 1, now(), now(), 'https://fly.storage.tigris.dev/mediadev/196e394e-17b5-417c-934c-12775ea689c1/index.jpeg', 2, 'index.m3u8', '597cb428-c830-45e5-834f-6bfb7b0e035a/index.m3u8', 'https://fly.storage.tigris.dev/mediadev/196e394e-17b5-417c-934c-12775ea689c1/og.png' ); ```
Lirianer commented 1 week ago

Expected is

12:55:05 -> 06 -> ... -> 21 -> 22 -> [discontinuity] -> 25 -> 26 -> 27 -> 28 -> 29 -> 30

but in my case (Linux & Chrome) it is

12:55:05 -> 06 -> ... -> 21 -> 22 -> [discontinuity] -> 29 -> 29 -> 29 -> 29 -> 29 -> 30

I inserted the video and am getting the expected, correct, result. Runnning the project from WSL/Ubuntu and watching from Windows/Firefox. The original demo recording was from macOS/Firefox also.

I've just installed chrome and could reproduce the problem on Windows. I'll test it later from macOS but it seems the issue is with Chrome at first glance, I'll test from Safari too to be sure.

Ok, tested the new code and the problem persists. I'll have another look at it

zcesur commented 1 week ago

Yeah it looks like a Chrome issue, maybe our playlist doesn't meet their spec somehow

Idk if this is helpful but the media tab shows audio buffering right after the discontinuity, which is somewhat weird because we use muxed_av where each CMAF segment contains both audio and video

Lirianer commented 1 week ago

I've tinkered for hours with the .m3u8 files but I couldn't find anything that fixes this. I've added and removed tags, tried different values and different video players (videojs and vidstack among others)

My only finding is that the problem seems to be with the first segment after the discontinuity tag, if I reversed the segments after the tag then instead of 8 seconds of static image it would be 1 (I'm testing with a different video with 2 segments of 8 and 1 seconds) Also in my test video the Audio Buffering issue did not appear, maybe because It was completely mute but I'm not sure.

I tried the mediastreamvalidator tool from apple and fixed some of the things that reported were bad with the files. I'm attaching the report for the example playlist you provided in the repo. From the Must Fix Issues I tried fixing 4 and 8, and messed a bit with 3. In my test video point 7 did not appear.

Something I'm not completely clear is if the video segment themselves have some datetime stamps, or something alike, which could maybe temper with how chrome process them after the tag, I did try removing the #EXT-X-PROGRAM-DATE-TIME tags and even replacing them with a daterange tag but it didn't work.

If you or someone has any clue of what may be wrong or any suggestion on what to investigate I'm grateful haha

validation_data.html.zip

zcesur commented 1 week ago

I see, thanks for digging into this! I'm also curious if using MPEG-TS segments and/or separate A/V tracks (as opposed to CMAF segments with muxed A/V as we do now) would help here

That being said I'm not sure if those would be readily compatible with our pipelines and I don't wanna bloat this PR with too many changes. So why don't you create a separate issue for the Chrome bug along with the repro and your findings, and I'll merge this as is once the avg bandwidth calculation is fixed

All this is super relevant & helpful for #73 too btw

Adding a /bonus $50 for your efforts & this issue being more complex than I initially thought

algora-pbc[bot] commented 1 week ago

A bonus of $50 has been added to the bounty by zcesur. @Lirianer: You will receive a total of $200 once you implement the requested changes and your PR is merged.

Lirianer commented 4 days ago

Ok! I've done the change to the avg_bandwidth, not really happy with going so many times to get the playlists files but I found it hard to avoid and not be messy, do tell if you see a better way.

I've created the issue #93 with the details of the problem and what we've discussed, although I'm really curious on why this happens and probably will do more research on it in the following week.

Thanks for the bonus, please tell me if you want me to change something else or if you have any idea to try and fix the issue!

zcesur commented 2 days ago

Looks good, thank you!