Recover the total length for formats that omit contentLength - #235
Conversation
|
Verified in production now.
|
8ad460f to
703a231
Compare
itag 18 carries no contentLength, and this stream requests ranges by query parameter rather than by header, so the response is a plain 200 whose Content-Length describes the chunk. PersistentHttpStream adopts that as the total, and playback ends at the first range boundary with an EOF inside the container reader. A zero-length ranged request does come back as a 206 carrying Content-Range, which holds the real total. Fall back to the previous behaviour if it does not yield a usable value.
703a231 to
b288db0
Compare
|
Confirmed in production with the exact code in this PR.
The range sequence walks to the true end of the file rather than stopping a buffer in: For contrast, variant 1 from the description (reading |
|
This doesn't sound right, though. Lavaplayer should have provisions to keep going even when the content length is signaled to be unknown. Your fix isn't a hack/workaround but, this would signal an issue on Lavaplayer's side that would need fixing as there is a specific value to denote unknown |
|
That is a fair reading, and I think you are right that something on the Lavaplayer side is off — but the problem is not that Lavaplayer fails to cope with an unknown length. It is that the length never stays unknown.
if (contentLength == Units.CONTENT_LENGTH_UNKNOWN) {
Header header = currentResponse.getFirstHeader("Content-Length");
if (header != null) {
contentLength = Long.parseLong(header.getValue());
}
}That is reasonable when the request was for the whole resource. But That is why the loss tracks I measured this rather than inferring it: the earlier variant of this PR read So the Lavaplayer-side fix would be to not adopt For this repo there is also a smaller thing in the same area: Happy to hold this PR while the Lavaplayer side is decided — I am carrying it locally either way and will report if a soak turns up anything. The measurement stands as posted: 298s of 298s with the code in this PR, against 291s without it. |
|
I think that's a fair assessment. I'm happy to merge this PR. I'm not too keen on making another request, specifically one to probe the content length, as it gives YouTube another angle to block us or break youtube-source but the alternative is having a track that doesn't play all the way through; neither is an ideal situation, but one has the benefit of fixing the other, and the risks are probably not greater than they are now. |
|
Following up on this — found the root cause and it's actually a lavaplayer issue, fixed there in lavaplayer#198. Measured end-to-end and the track plays in full now. #237 reverts this once that's merged. Thanks for the pointer! |
Fixes #234.
Cause
extractFormatlets itag 18 through without acontentLengthwhile skipping every other format missing one, and itag 18 is the only format left when a client's response is SABR-gated — so on those videos it is the whole of playback.PersistentHttpStreamnormally recovers a missing length by adoptingContent-Lengthfrom the first response. ButYoutubePersistentHttpStreamsetsuseHeadersForRange()tofalseand carries the range in the query instead, so the response is a plain 200 whoseContent-Lengthdescribes the chunk, not the resource. The stream ends up believing it is one buffer long and stops at the first range boundary:The loss lines up with
BUFFER_SIZE(11862014):8CFh_-qtzegis 12164812 bytes, so one buffer is 97.5% of it, against 291s of 298s (97.7%) actually played.8CFh_-qtzegi2eeqThRBlQChange
Ask for the total with a zero-length ranged request, which does come back as a 206 carrying
Content-Range. Only for formats that arrive without a length, and it falls back to the previous behaviour whenever the probe does not yield a usable value.Two smaller variants I tried first, and why they do not work
You may reach for these, so recording the results:
Content-Rangeoff the existing response (overridecreateContentInputStream). Does not work — the range goes in the query, so YouTube answers 200 with noContent-Rangeat all. Deployed and measured: still 292s of 298s.getConnectRequest). Does not compile —getConnectRequest()is private in the released lavaplayer, though it isprotectedon lavaplayer main. Would become viable on a future bump.If you would rather have the probe live in
YoutubePersistentHttpStreamthan inYoutubeAudioTrack, that reads better to me too and I am happy to move it — I left it here only because that is the form I have measured.Verification
Measured in production: the two videos above played 299s/298s and 289s/289s with no EOF in the container reader, against 291s and 277s on the same build without the change, and 25 distinct tracks logged a recovered length. That was with a slightly more defensive version of this same probe; the form in this PR is deployed now and I will confirm it here once it has played a full track.