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

fix: Fix errors with TS segments on Chromecast #4543

Merged
merged 7 commits into from
Oct 6, 2022

Conversation

JulianDomingo
Copy link
Contributor

@JulianDomingo JulianDomingo commented Oct 4, 2022

On Chromecast devices, TS-based streams tend to be more susceptible to failures related to the Cast platform's MSE implementation.

One of these failures was observed when Shaka Player updates SourceBuffer#timestampOffset within MediaSourceEngine#appendBuffer():

  1. To extract a timestamp from MediaSource to align text segments:
    sourceBuffer.timestampOffset = originalOffset;
  2. To re-align segments as a result of an unbuffered seek or automatic adaptation:
    this.enqueueOperation_(
    contentType,
    () => this.setTimestampOffset_(contentType, timestampOffset));

The Cast platform throws the following error:

...
"stack":"Error: Failed to set the 'timestampOffset' property on 'SourceBuffer': The timestamp offset
may not be set while the SourceBuffer's append state is 'PARSING_MEDIA_SEGMENT'.
...

One way of resolving this is to reset the SourceBuffer's parser state, which in turn resets the append state from PARSING_MEDIA_SEGMENT => WAITING_FOR_SEGMENT: https://www.w3.org/TR/media-source-2/#sourcebuffer-reset-parser-state.

On Cast devices, the issue is reproduced consistently when Shaka switches to a new variant. There are no internally queued MSE operations in MediaSourceEngine when the error is thrown. Based on these observations, the input buffer passed to MediaSourceEngine seems to be an "incomplete" media segment, meaning the SourceBuffer#appendBuffer() call exits when the append state is still PARSING_MEDIA_SEGMENT (Step 6.4 in W3C specs) hence the error.

This is also suggested as a root cause from a Chromium engineer, with one suggested solution to call abort() beforehand (source). Since Shaka Player already does preemptive abort() calls when setting stream properties, this will do the same thing.

@avelad avelad added type: bug Something isn't working correctly component: HLS The issue involves Apple's HLS manifest format platform: Cast Issues affecting Cast devices labels Oct 4, 2022
@avelad avelad added this to the v4.3 milestone Oct 4, 2022
@avelad avelad requested review from theodab and joeyparrish October 4, 2022 18:52
@avelad avelad added the priority: P2 Smaller impact or easy workaround label Oct 4, 2022
@JulianDomingo JulianDomingo self-assigned this Oct 4, 2022
test/media/media_source_engine_unit.js Outdated Show resolved Hide resolved
@@ -649,6 +649,50 @@ describe('MediaSourceEngine', () => {

expect(videoSourceBuffer.timestampOffset).toBe(0.50);
});

it('calls abort before setting timestampOffset', async () => {
const delay = async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this does both a delay and an updateend() event, maybe call it simulateUpdate() or something? Reading the test body without the definition of delay(), you might expect it to only wait for time to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, updated!

test/media/media_source_engine_unit.js Show resolved Hide resolved
@joeyparrish joeyparrish added priority: P1 Big impact or workaround impractical; resolve before feature release and removed priority: P2 Smaller impact or easy workaround labels Oct 4, 2022
theodab
theodab previously approved these changes Oct 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

Incremental code coverage: 100.00%

@joeyparrish joeyparrish marked this pull request as ready for review October 4, 2022 21:28
joeyparrish
joeyparrish previously approved these changes Oct 5, 2022
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM without the Cast-specific regression test. If we can get that in, great. If not, this is still an important fix that we should get out to partners.

@joeyparrish joeyparrish changed the title fix: Resolve Chromecast-based appendBuffer() failures when appending TS segments. fix: Fix errors with TS segments on Chromecast Oct 5, 2022
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test case fails on Chromecast without the fix, and passes with it. Nicely done!

@joeyparrish joeyparrish merged commit 593c280 into shaka-project:main Oct 6, 2022
JulianDomingo added a commit that referenced this pull request Oct 6, 2022
On Chromecast devices, TS-based streams tend to be more susceptible to
failures related to the Cast platform's MSE implementation.

One of these failures was observed when Shaka Player updates
`SourceBuffer#timestampOffset` within
`MediaSourceEngine#appendBuffer()`:
1. To extract a timestamp from `MediaSource` to align text segments:
https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L657
2. To re-align segments as a result of an unbuffered seek or automatic
adaptation:
https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L684-L686

The Cast platform throws the following error:
```
...
"stack":"Error: Failed to set the 'timestampOffset' property on 'SourceBuffer': The timestamp offset
may not be set while the SourceBuffer's append state is 'PARSING_MEDIA_SEGMENT'.
...
```

One way of resolving this is to reset the SourceBuffer's parser state,
which in turn resets the append state from `PARSING_MEDIA_SEGMENT` =>
`WAITING_FOR_SEGMENT`:
https://www.w3.org/TR/media-source-2/#sourcebuffer-reset-parser-state.

On Cast devices, the issue is reproduced consistently when Shaka
switches to a new variant. There are no internally queued MSE operations
in `MediaSourceEngine` when the error is thrown. Based on these
observations, the input buffer passed to `MediaSourceEngine` seems to be
an "incomplete" media segment, meaning the `SourceBuffer#appendBuffer()`
call exits when the append state is still `PARSING_MEDIA_SEGMENT` ([Step
`6.4` in W3C
specs](https://www.w3.org/TR/media-source-2/#sourcebuffer-segment-parser-loop))
hence the error.

This is also suggested as a root cause from a Chromium engineer, with
one suggested solution to call abort() beforehand
([source](https://bugs.chromium.org/p/chromium/issues/detail?id=766002#c3)).
Since Shaka Player already does preemptive abort() calls when [setting
stream
properties](https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L836-L842),
this will do the same thing.

Co-authored-by: Joey Parrish <[email protected]>
JulianDomingo added a commit that referenced this pull request Oct 6, 2022
On Chromecast devices, TS-based streams tend to be more susceptible to
failures related to the Cast platform's MSE implementation.

One of these failures was observed when Shaka Player updates
`SourceBuffer#timestampOffset` within
`MediaSourceEngine#appendBuffer()`:
1. To extract a timestamp from `MediaSource` to align text segments:
https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L657
2. To re-align segments as a result of an unbuffered seek or automatic
adaptation:
https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L684-L686

The Cast platform throws the following error:
```
...
"stack":"Error: Failed to set the 'timestampOffset' property on 'SourceBuffer': The timestamp offset
may not be set while the SourceBuffer's append state is 'PARSING_MEDIA_SEGMENT'.
...
```

One way of resolving this is to reset the SourceBuffer's parser state,
which in turn resets the append state from `PARSING_MEDIA_SEGMENT` =>
`WAITING_FOR_SEGMENT`:
https://www.w3.org/TR/media-source-2/#sourcebuffer-reset-parser-state.

On Cast devices, the issue is reproduced consistently when Shaka
switches to a new variant. There are no internally queued MSE operations
in `MediaSourceEngine` when the error is thrown. Based on these
observations, the input buffer passed to `MediaSourceEngine` seems to be
an "incomplete" media segment, meaning the `SourceBuffer#appendBuffer()`
call exits when the append state is still `PARSING_MEDIA_SEGMENT` ([Step
`6.4` in W3C
specs](https://www.w3.org/TR/media-source-2/#sourcebuffer-segment-parser-loop))
hence the error.

This is also suggested as a root cause from a Chromium engineer, with
one suggested solution to call abort() beforehand
([source](https://bugs.chromium.org/p/chromium/issues/detail?id=766002#c3)).
Since Shaka Player already does preemptive abort() calls when [setting
stream
properties](https://github.com/shaka-project/shaka-player/blob/3d0f752c7d0677f750dbbd9bcd2895358358628f/lib/media/media_source_engine.js#L836-L842),
this will do the same thing.

Co-authored-by: Joey Parrish <[email protected]>
JulianDomingo pushed a commit that referenced this pull request Oct 7, 2022
🤖 I have created a release *beep* *boop*
---


##
[4.1.5](v4.1.4...v4.1.5)
(2022-10-07)


### Bug Fixes

* allow build without text
([#4506](#4506))
([1db6265](1db6265))
* allow the playback on platforms when low latency APIs are not
supported
([#4485](#4485))
([55d1390](55d1390))
* check for negative rows before moving
([#4510](#4510))
([31abae3](31abae3)),
closes
[#4508](#4508)
* Filter unsupported H.264 streams in Xbox
([#4493](#4493))
([1ecede6](1ecede6))
* Fix choppy HLS startup
([#4553](#4553))
([1675bff](1675bff)),
closes
[#4516](#4516)
* Fix errors with TS segments on Chromecast
([#4543](#4543))
([15a1c60](15a1c60))
* Fix hang when seeking to the last segment
([#4537](#4537))
([72a119d](72a119d))
* Fix HLS dynamic to static transition
([932d37c](932d37c))
* Fix HLS dynamic to static transition
([#4483](#4483))
([932d37c](932d37c)),
closes
[#4431](#4431)
* Fix in-band key rotation on Xbox One
([#4478](#4478))
([5a8f09c](5a8f09c)),
closes
[#4401](#4401)
* Respect existing app usage of Cast SDK
([#4523](#4523))
([9c3a494](9c3a494)),
closes
[#4521](#4521)
* **ttml:** Default TTML background color to transparent if unspecified
([#4496](#4496))
([16da1e7](16da1e7)),
closes
[#4468](#4468)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
JulianDomingo pushed a commit that referenced this pull request Oct 7, 2022
🤖 I have created a release *beep* *boop*
---


##
[4.2.2](v4.2.1...v4.2.2)
(2022-10-07)


### Bug Fixes

* allow build without text
([#4506](#4506))
([7e93720](7e93720))
* allow the playback on platforms when low latency APIs are not
supported
([#4485](#4485))
([cf8c857](cf8c857))
* check for negative rows before moving
([#4510](#4510))
([23f39d7](23f39d7)),
closes
[#4508](#4508)
* Filter unsupported H.264 streams in Xbox
([#4493](#4493))
([914a08a](914a08a))
* Fix choppy HLS startup
([#4553](#4553))
([950ce69](950ce69)),
closes
[#4516](#4516)
* Fix errors with TS segments on Chromecast
([#4543](#4543))
([8204db6](8204db6))
* Fix hang when seeking to the last segment
([#4537](#4537))
([3d6c768](3d6c768))
* Fix HLS dynamic to static transition
([d9ecbf3](d9ecbf3))
* Fix HLS dynamic to static transition
([#4483](#4483))
([d9ecbf3](d9ecbf3)),
closes
[#4431](#4431)
* Fix in-band key rotation on Xbox One
([#4478](#4478))
([bc0a588](bc0a588)),
closes
[#4401](#4401)
* Missing AES-128 key of last HLS segment
([#4519](#4519))
([2c2677f](2c2677f)),
closes
[#4517](#4517)
* Respect existing app usage of Cast SDK
([#4523](#4523))
([3db2568](3db2568)),
closes
[#4521](#4521)
* **ttml:** Default TTML background color to transparent if unspecified
([#4496](#4496))
([0b5c985](0b5c985)),
closes
[#4468](#4468)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: HLS The issue involves Apple's HLS manifest format platform: Cast Issues affecting Cast devices priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants