xbmc / inputstream.adaptive

kodi inputstream addon for several manifest types
Other
453 stars 241 forks source link

MPD manifest error causes playback delay #433

Closed dagwieers closed 4 years ago

dagwieers commented 4 years ago

We discovered that an incorrect MPEG-DASH manifest, including a subtitle that gives HTTP 415 (Unsupported Media Type) cause IA to retry downloading 10 times causing playback delays from 10 to 20 seconds.

Maybe if a subtitle track causes HTTP 415, it should not be retried, but skipped.

See add-ons/plugin.video.vrt.nu#735 for the details.

matthuisman commented 4 years ago

So, to confirm -we are happy with the retry logic then (but maybe less retries)? And now agree 415 is retried by everyone.

I guess the real fix would be IA using different threads for video / audio / subs. That's a big job. But i think the GSOC adaptive may be using that to fetch future segments etc, so may sort it out

Maybe we can consider my idea of just skipping retries of subs alltogether? It'll try the next sub next time it fetches mpd anyway.

dagwieers commented 4 years ago

I never was against retries, and I still think HTTP 415 is unrecoverable. We still don't know why they return 415 in this case, but at least THEOplayer doesn't mind at all. The reference player gives up, but doesn't impact playback.

So the best fix would be something that doesn't affect playback IMO. We won't have subtitles with the current behaviour (or if we drop retries) so the behaviour is suboptimal in any case.

matthuisman commented 4 years ago

The players do MIND. They keep retrying the same file. They are not skipping retries on 415

Yes, you get the playing result - but you can't just ignore what they are doing. They are retrying - but it's in different threads. IA is doing the same thing - but only single thread.

So, we can either use different threads - or we can skip retries. We can't have both

I vote for now to skip retries of subtitles altogether. We don't want subs affecting our playback. Just like JavaScript in a website, it should be an enhancement - not a requirement. However, that was thrown out the window with web 2.0.

Or maybe subs only retry once. and video / audio retry 3 times.

Something like this I like more than specifically looking for 415.

But you say the subs may start working - so you do want it to retry eventually? Does IA keep retrying after eventually starting playback (eg. after redownloading mpd)?

Have you asked the provider if they can just return a subs file with no subs in it? Seems that would be the correct approach for subs that come and go. That would make their MPD work in all the players. It also saves all the extra requests on their server from their own player :)

It's listed in the MPD - as far as IA is concerned - it should be there - thats why it retries. IA shouldn't need to think some MPD content is "optional".

dagwieers commented 4 years ago

Just as a matter of due diligence I reported the playback issues with JW Player and FlowPlayer upstream as well:

So a small recap of all test players (from best to worse wrt. handling this issue):

Player Playback Behaviour
THEOplayer Works fine No retries, gets newer segments
(You need to enable subtitles in the player)
Shaka Player Works fine Retries once, gets newer segments, and reports
(You need to enable subtitles in the player)
DASH reference player Works fine Retries 3 times, reports, stops subs
(Subtitles are enabled by default)
Bitmovin Works fine Retries twice, gets next segment indefinitely
(You need to enable subtitles in the player)
Akamai Stutters, then works fine Keeps retrying same file indefinitely
(Subtitles are enabled by default)
JW Player Fails immediately (Subtitles are enabled by default)
FlowPlayer Fails immediately (Subtitles are enabled by default)

The list of test players is found here: https://github.com/add-ons/best-practices/wiki/Streaming-issues

dagwieers commented 4 years ago

The players do MIND. They keep retrying the same file. They are not skipping retries on 415

I meant they don't mind the 415 (as in give up), but they don't retry the same file. THEOplayer handles it best as it continues supporting without affecting the playback or user and eventually picks up subtitles when they are available. IA both affects playback, as well as give up (on the subtitles). At least the playback eventually continues without any further issues, but without subtitles when they would be available.

Something like this I like more than specifically looking for 415.

As I already mentioned, yes that was suggested. But again, you are looking at this from the perspective of this specific erratic behaviour of a stream that should not be, but I am looking from the perspective of streams we cannot influence and a player that should behave better despite errors. (resilience/reliability)

So even if this specific VOD provider would have fixed this, the player should handle such cases better.

matthuisman commented 4 years ago

And what is better to you?

How should IA handle errors? You want retries but don't want the delay they introduce?

Only way I see to get that is threads. But we need to wait for the initial video / audio (cant thread them) But we don't want to wait for initial subs.

It seems to me - subs need to be treated differently.

With your file, does IA keep trying the subs during playback?

dagwieers commented 4 years ago

For the subtitle track retries are probably not very useful, but doing the next segment would be useful. So I guess the proposed PR by @mediaminister is exactly what we want if we cannot have threads and if it picks up newer segments.

So maybe the only thing missing from the PR is some guidance to why it was added, what would be better (threads?).

matthuisman commented 4 years ago

I think it's fine if we remove the check for 415 and instead check for any errror (not OK response) as I said many comments back :)

I think != 200 may be wrong as you can have partial response codes etc.

Check if response code doesn't start with 2 should do it I think. Redirects should already be handled by the time it gets here in code.

So, if it's a subtitle and response code not start with 2: don't retry.

UPDATE: I've chatting with @glennguy and he thinks there is a download thread in IA. If it's just a single thread with a queue that won't help. Unless instead of retrying, put the failed file back at the end of the queue to retry last. Keeps getting put back into queue for X amount of times (retries) That way, the first video and audio will get done, playback starts and subs keep retrying.

But if we can spawn a new thread for each download - then that should fix this issue. Or, maybe there is something making IA wait for subs further on. Or maybe it's Kodi that waits.

Did you confirm IA keeps trying the subs after playback starts? (which is what we want) But maybe just not 10 times every time

dagwieers commented 4 years ago

I cannot confirm or deny because I have no environment to build Kodi at this time. Maybe @mediaminister can confirm in the morning.

matthuisman commented 4 years ago

don't need to build kodi. just have debug logging with curl enabled. (actually, don't need curl enabled - IA will show the error)

Then playback the mpd. You will be able to see the initial 10 retries. Then hopefully see it keep retying every-time it re-downloads the mpd.

I'll check now :)

Also, what happens if you start the stream with subs disabled? Does it start straight away? If so - maybe it's more a kodi issue. Its waiting for subs if they are enabled before starting playback.

Anyway, this issue has been a great way to start a discussion on retry logic in general and how things can be handled nicer. In conclusion - dont let subs affect playback.

And by the way - it's pretty cool your provider supports and encourages Kodi and your add-on. Here in NZ / AU - it's usually no word until a dmca takedown (why I no longer public host my addon repos)

dagwieers commented 4 years ago

Ah, I figured you meant mediaminister's branch, because it affects the behaviour.

Yes, we have subtitles enabled by default, but this is by choice.

Again, we are not looking for a fix for our issue. We are looking into fixing Kodi's behaviour. In our add-on we switched to HLS for this specific stream which is not affected by the delay.

dagwieers commented 4 years ago

By coincidence I find this in my logs of 2 hours ago:

2020-05-11 23:08:20.078 T:1027597184   ERROR: AddOnLog: InputStream Adaptive: Decrypt Sample returns failure!
2020-05-11 23:09:56.450 T:985498496   ERROR: Previous line repeats 1 times.
2020-05-11 23:09:56.450 T:985498496   ERROR: AddOnLog: InputStream Adaptive: Download https://live-cf-vrt.akamaized.net/groupc/live/8edf3bdf-7db3-41c3-a318-72cb7f82de66/live.isml/dash/live-text_track_0_dut=1000-1589231389759.dash failed with error: 404
2020-05-11 23:09:57.888 T:1027597184   ERROR: AddOnLog: InputStream Adaptive: Decrypt Sample returns failure!
2020-05-11 23:10:34.771 T:985498496   ERROR: Previous line repeats 2 times.
2020-05-11 23:10:34.771 T:985498496   ERROR: AddOnLog: InputStream Adaptive: Download https://live-cf-vrt.akamaized.net/groupc/live/8edf3bdf-7db3-41c3-a318-72cb7f82de66/live.isml/dash/live-text_track_0_dut=1000-1589231428159.dash failed with error: 404
2020-05-11 23:10:51.362 T:1027597184   ERROR: AddOnLog: InputStream Adaptive: Decrypt Sample returns failure!
2020-05-11 23:12:12.709 T:985498496   ERROR: Previous line repeats 4 times.
2020-05-11 23:12:12.709 T:985498496   ERROR: AddOnLog: InputStream Adaptive: Download https://live-cf-vrt.akamaized.net/groupc/live/8edf3bdf-7db3-41c3-a318-72cb7f82de66/live.isml/dash/live-text_track_0_dut=1000-1589231526079.dash failed with error: 404
2020-05-11 23:13:06.994 T:1027597184   ERROR: AddOnLog: InputStream Adaptive: Decrypt Sample returns failure!
2020-05-11 23:14:34.706 T:985498496   ERROR: Previous line repeats 2 times.
2020-05-11 23:14:34.706 T:985498496   ERROR: AddOnLog: InputStream Adaptive: Download https://live-cf-vrt.akamaized.net/groupc/live/8edf3bdf-7db3-41c3-a318-72cb7f82de66/live.isml/dash/live-text_track_0_dut=1000-1589231668159.dash failed with error: 404
2020-05-11 23:14:45.801 T:1027597184   ERROR: AddOnLog: InputStream Adaptive: Decrypt Sample returns failure!
2020-05-11 23:15:18.895 T:985498496   ERROR: Previous line repeats 18 times.

Here we get 404 errors on subtitle tracks (this is a live tv channel that does have subtitle tracks) and here it doesn't seem to be retrying. Although it doesn't seem to affect playback either (possibly because it started fine with subtitles). Retries here are useless too.

This is the same VOD provider BTW...

matthuisman commented 4 years ago

ok, so 100% not just check for 415 then.

That is fine though - all the big players would done the same thing. If it didn't affect playback - your happy right? Only downside is log spam. It needs to try the subs in case they do come back. With a live edge - it may just be timing - so good to retry.

Good to see IA is at least trying the next text segment (not the same over and over) Just needs to only do 1 try for subs though you reckon? Think we need to stop saying retry - as thats ambiguous. 0 retries = 1 try then 0 retry? So let's start saying "tries" as in how many total tries for that file. For subs - we only want a single try?

And, you can download IA builds from PR's that are automatically built. eg. https://jenkins.kodi.tv/blue/organizations/jenkins/peak3d%2Finputstream.adaptive/detail/PR-441/1/artifacts

dagwieers commented 4 years ago

Yes, I already added my thoughts to PR #441.

dagwieers commented 4 years ago

@matthuisman I updated the matrix of all player's behaviour here: https://github.com/peak3d/inputstream.adaptive/issues/433#issuecomment-626994942

And as you can see the detailed behaviour is also erratic for some players, despite not being affected in playback. Only THEOplayer handles this best IMO (if you consider 415 unrecoverable, which I think it is).

matthuisman commented 4 years ago

@dagwieers Try this: https://github.com/peak3d/inputstream.adaptive/pull/445

dagwieers commented 4 years ago

I found this while digging into HTTP 415: https://docs.unified-streaming.com/documentation/package/usage.html#fmp4-415

This gives alternative meanings to existing HTTP errors in a streaming context.

matthuisman commented 4 years ago

why are we back on 415... You've already seen sometimes you get 404 errors as well. And we have also confirmed a lot of players retry on it as well (don't specifically look for 415)

Its still the wrong error they are returning. It's not invalid format or unsupported media - it's simply the file doesn't exist.

It means nothing - its just an obscure random error they have chosen to return for some reason. If they return the correct 404 - behaviour everywhere will stay the same.

dagwieers commented 4 years ago

@matthuisman Because we did not understand why we got a 415 Unsupported Media Type, and this confirms that HTTP 415 has different meanings, at least for Unified Streaming, but likely for others as well

If you don't want to get this info, I suggest you disable notifications for this PR, because I am not writing this just for you, this is a log of our findings in general. Maybe someone else finds this useful.

It means nothing - its just an obscure random error they have chosen to return for some reason.

Point being, it is not an obscure random error. Thanks for reading that link.

matthuisman commented 4 years ago

the developer even told you thats a random error they chose. So you know it's just them doing it. It's not some sort of secret standard that no one knows about except this one crowd.

I read it - but they are using it incorrectly. There file DOESNT EXIST. Its not invalid format, its not wrong media type. Its a 404 - Does not exist!

415 would make sense if we requesting a mp4 stream and they only have a ts stream.

The fact you keep going away and trying to find different definitions for 415 is irrelevant as its the wrong error to return in the first place if the file doesnt exist.

And I am checking out. You just seem to ignore any logic that doesn't fit your single stream. Lets look for 415 errors because one stream uses it incorrectly.

I thought we already worked out to just check for any errors. But back on 415 again. That is circles.

Those players you say work will just be checking for any error - not 415. Open their source code and look for 415. It won't be there. The error code is irrelevant.

dagwieers commented 4 years ago

@matthuisman We have never talked to a developer, and the guy who you think is a developer but is not, is not working on any products either, did not say it was a random error. In fact he said it was a very specific error:

Since the subtitle track is not (always) present, the origin can't provide the requested data, and returns the 415 status that you observed. From the point of view of our origin partner, this is expected behaviour.

The player (TheoPlayer) we use on our official sites/apps ("touchpoints") handles this 415 as meaning "no subtitles present", and starts to play anyway. The players you tested (ffmpeg, jwplayer) behave differently. (Note that I'm trying to avoid pointing fingers at who's right or wrong)

I am not questioning anything we already established, please stop claiming I do.

dagwieers commented 4 years ago

And could you please stop arguing and maybe disable notifications? It is getting tiresome.

matthuisman commented 4 years ago

fine with me. I look forward to seeing your fix

mediaminister commented 4 years ago

I looked into this again, 415 is returned because there are no subtitle samples created at the streaming backend of the content provider, the streaming provider Unified Origin states the following in their documentation:

In the error log of the webserver you will find 'FMP4_415 No samples for fragment', because the webserver tried to create a fragment and could not find samples to do so

I agree with @matthuisman that the content provider should fix this:

If they have subs that come and go - then return files with no subs in them. This would allow subs to start half way through a segment as well.

Providing empty subtitle fragments is indeed the only right solution here.

Have you asked the provider if they can just return a subs file with no subs in it?

No, but the content provider will try to implement this:

but we're looking in to always adding a (possibly empty) subtitle track. If this can be done easily, we'll do that.

Its still the wrong error they are returning. It's not invalid format or unsupported media - it's simply the file doesn't exist.

Indeed, this is an internal error between the content provider and the streaming provider. They shouldn't return 415 to the clients but instead a 404.

I'll close my pull request, adding more complexity to IA for a problem that is caused by the content provider is not a good idea.

dagwieers commented 4 years ago

Ok, I will open a PR to make AI more resilient. If Unified Origin would be returning a 404 instead of 415, IA has the exact same problem. And other players handle this more gracefully.

peak3d commented 4 years ago

Wow, this is a long discussion here :-) I suggest that IA callbacks the addon with the HTTP error code and your addon has the possibility to select what to do.

dagwieers commented 4 years ago

@peak3d I don't think that is necessary, as the add-on won't be able to implement what is ideal either. See #446 for the details. I am not sure if anyone wants a configurable behaviour for subtitle fragment errors.

dagwieers commented 4 years ago

@basilgello If your issue has not been fixed yet, could you open a new ticket for this?

basilgello commented 4 years ago

@basilgello If your issue has not been fixed yet, could you open a new ticket for this?

Sure, will open today,