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

Update dash chunk source to handle in-progressive recording dash stream. #528

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

zgzong
Copy link
Contributor

@zgzong zgzong commented Jul 18, 2023

Using calculated segments timeline duration to compare with period duration to update whether period has reach it the end of stream.

@tonihei
Copy link
Collaborator

tonihei commented Jul 19, 2023

I did some testing with some multi-period DASH streams and it seems this isn't entirely safe. The problem is that many streams don't declare the overall duration accurately enough that this check always works. Sometimes, the last segment ends a few hundred milliseconds before the declared end of the period for example and the new check here would prevent the player from ever considering this period ended. In addition, we should always assume that a period that isn't the last one in the manifest is ended even if this condition isn't true. I'll make some additional changes to the PR (you'll see my commits here) and then merge it.

@zgzong
Copy link
Contributor Author

zgzong commented Jul 19, 2023

It seems a common issue for Vod stream. The declared MPD@ mediaPresentationDuration is not accurate, it is bigger than one adaptationSet's duration. The stream packager may ignore the last hundreds milliseconds because it is too short to generate a segment.
The multiple periods did meet same issue for these periods which ends already.

@tonihei
Copy link
Collaborator

tonihei commented Jul 19, 2023

The multiple periods did meet same issue for these periods which ends already.

Are you saying that the case you are trying to solve also has multiple periods that all have their duration set but are not yet available? Or is this more a generic comment agreeing with my change?

The commit I was talking is uploaded (see above), so please take a look and leave any comments if needed.

@zgzong
Copy link
Contributor Author

zgzong commented Jul 19, 2023

The multiple periods did meet same issue for these periods which ends already.

Are you saying that the case you are trying to solve also has multiple periods that all have their duration set but are not yet available? Or is this more a generic comment agreeing with my change?

The commit I was talking is uploaded (see above), so please take a look and leave any comments if needed.

The issue we are facing is single period live stream.
My original PR miss out the multi-periods cases, from theory they are same as the issue we meet in the vod streams.
Vote up for your change.

@tonihei
Copy link
Collaborator

tonihei commented Jul 19, 2023

Thanks for the confirmation! We'll work on submitting the change internally and then you should see the PR marked as merged soon.

zgzong and others added 2 commits August 8, 2023 17:42
Using calculated segments timeline duration to compare with period duration to update whether period has reach it the end of stream.
Periods that are not the last in the manifest are always ended
and non-dynamic periods are also always ended by definition.

Also, some periods may not be able to declare their final duration
accurately and we need to account for some inaccuracy. This can be
done by testing whether no further full segment within the duration
can be expected.
@tianyif tianyif merged commit ef54364 into androidx:main Aug 10, 2023
1 check passed
tianyif added a commit that referenced this pull request Aug 14, 2023
PiperOrigin-RevId: 554869426
(cherry picked from commit ef54364)
@androidx androidx locked and limited conversation to collaborators Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants