Shaka-player: 1h+ DASH videos on Xbox One throw Error 4001

[tylermachen]

Related to #2186

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
v2.5.9

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
Custom app

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
N/A

For embedded devices (smart TVs, etc.), what model and firmware version are you using?
Xbox One, OS v.10.0.18363.9135 February 2020

What are the manifest and license server URIs?

Any DASH manifest which is big enough (1K+ lines) will work, this is the manifest which was provided by Shaka Player Team in previous issue #2186
https://storage.googleapis.com/wvtemp/modmaker/long-segment-timeline.mpd

What did you do?

I tried to play the video with a duration of over 1h on the Xbox One. In our case, our manifests include SegmentTimeline tag.

What did you expect to happen?
The video will start playback.

What actually happened?


Player throws an error when we try to play the video:

Can not load manifest: shaka.util.Error { "severity": 2, "category": 4, "code": 4001, "data": [ "https://example-site.com/master.mpd" ], "handled": false }

Which is DASH_INVALID_XML, 4001, The DASH Manifest contained invalid XML markup.

Also, we found that this issue is only reproducible with Compiled (Release) code, it’s playing fine in the Compiled (Debug) and Uncompiled modes. In the compiled code, the line which is testing the device buffer size is missing https://github.com/google/shaka-player/blob/master/lib/util/string_utils.js#L220

[theodab]

Yeah, it looks like that method is getting really messed up by the Closure Compiler. In the output, it straight-up just... makes the Uint8Array, doesn't assign it to a variable, and returns true. So, yes, it's skipping the line of code that actually detects whether or not we can handle that chunk size.

I'll see if I can get the compiler to behave.

[joeyparrish]

Yet another reason to get the compiler upgraded ASAP. We've put it off for too long.

Can we create an integration test that would catch this using the compiled library?

[theodab]

I've figured out the exact problem; we were testing to see if the given chunk size was supported by performing an operation on a Uint8Array of that size, and seeing if it throws any errors. However, we only ever referenced the result of that operation in an assert. So in release mode, where asserts are disabled, the compiler was just compiling out the now-seemingly-useless operation.
So this is a mistake that a better compiler might also make.

Luckily, it's a pretty simple fix. I'll see if I can make an integration test for it, too.

[theodab]

Sorry, I should have been more clear. I already have a fix for this, I was just working on a test to submit with it.

[tylermachen]

No problem. When do you anticipate the fix will be released?

[joeyparrish]

Hopefully this week. I raised an issue in review about our regression testing, and we're working to make sure this subtle, tricky-to-notice, platform-specific bug won't come back.

[theodab]

Okay, the fix has now been released. Sorry about the delay, the regression test ended up being unexpectedly complicated!
If you have any further problems, feel free to bring them up.

[tylermachen]

@theodab Thanks for the fix! When do you anticipate this will be made available in an official release?

[joeyparrish]

I've been working on cherry-picks for the v2.5.10 release. I hope to have it completed this week.