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

Period flattening error on DASH manifests with image AdaptationSet #3383

Closed
jennyzhua opened this issue May 3, 2021 · 7 comments
Closed
Assignees
Labels
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
Milestone

Comments

@jennyzhua
Copy link

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

What link can we use to reproduce this?
manifest sent in email

What version of Shaka Player are you using?
v3.1.0-uncompiled

What browser and OS are you using?
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/89.0.4389.128 Safari/537.36

What did you do?
Play the manifest in the demo app

What did you expect to happen?
Playback starts successfully

What actually happened?
Encountered Shaka Error MANIFEST.PERIOD_FLATTENING_FAILED ()

@avelad
Copy link
Member

avelad commented May 5, 2021

After this PR #3365, the tileLayout information will be entered into the segments, so in theory DASH could be adapted to use the same format and eliminate this limitation.

@joeyparrish
Copy link
Member

I think the short-term solution needs to be a fix to period-flattening to ignore images. That could be cherry-picked to the existing v3.0.x and v3.1.x release branches and can appear in a bugfix release, whereas any pending feature PRs are going to first appear in a future feature release.

@joeyparrish joeyparrish added type: bug Something isn't working correctly priority: P1 Big impact or workaround impractical; resolve before feature release and removed needs triage labels May 7, 2021
@shaka-bot shaka-bot added this to the v3.2 milestone May 7, 2021
@avelad
Copy link
Member

avelad commented Jun 22, 2021

@jennyzhua Can you copy the manifest here (text) so I can work on the fix?

@joeyparrish joeyparrish modified the milestones: v3.2, v3.3 Jul 7, 2021
@joeyparrish joeyparrish self-assigned this Jul 9, 2021
@joeyparrish
Copy link
Member

@jennyzhua, we have the content URL you shared privately, and I am working on this issue now. The issue seems to be particular to having two different resolutions of images per period. You may be able to work around it by removing one on the backend or at runtime by manipulating the manifest in the client. I hope to have a solution soon, though.

Thanks for your patience!

@joeyparrish
Copy link
Member

I have a fix working now in manual testing, and regression tests to cover this case as well. The changes are in review now, and will be in our next release.

@avelad
Copy link
Member

avelad commented Jul 12, 2021

@joeyparrish After closing this issue, will you release the version 3.2? What day?

shaka-bot pushed a commit that referenced this issue Jul 12, 2021
Matching image streams across periods was not working correctly with
multiple streams per period.  Matching was done on MIME type only,
meaning there was no way to differentiate multiple streams.
Resolution is a better signal, so this uses the same logic as for
video resolution in thumbnail resolution matching.

This also adds extra error logs that helped track down the issue, and
fixes some comment typos.

Issue #3383

Change-Id: I067de59c222a165d58926646f53fa61862853377
shaka-bot pushed a commit that referenced this issue Jul 12, 2021
We filter out duplicate streams for all other types, so we should do
this for images, as well.

This also updates all PeriodCombiner test cases to include the
imageStreams property.

Issue #3383

Change-Id: I63355f85d4e12bd25c1cadce29a040d17c4e7d61
shaka-bot pushed a commit that referenced this issue Jul 12, 2021
When loading offline content, we should load the image streams as
well.

Issue #3383

Change-Id: I8cdd3d1ade188b7b2278a0eac5510a968229c6fc
@joeyparrish
Copy link
Member

I'm working on cherry-picks for the releases now. v3.2.0 and v3.1.2 at least, and v3.0.13 and v2.5.24 if necessary.

joeyparrish added a commit that referenced this issue Jul 13, 2021
Matching image streams across periods was not working correctly with
multiple streams per period.  Matching was done on MIME type only,
meaning there was no way to differentiate multiple streams.
Resolution is a better signal, so this uses the same logic as for
video resolution in thumbnail resolution matching.

This also adds extra error logs that helped track down the issue, and
fixes some comment typos.

Issue #3383

Change-Id: I067de59c222a165d58926646f53fa61862853377
joeyparrish added a commit that referenced this issue Jul 13, 2021
We filter out duplicate streams for all other types, so we should do
this for images, as well.

This also updates all PeriodCombiner test cases to include the
imageStreams property.

Issue #3383

Change-Id: I63355f85d4e12bd25c1cadce29a040d17c4e7d61
joeyparrish added a commit that referenced this issue Jul 13, 2021
When loading offline content, we should load the image streams as
well.

Issue #3383

Change-Id: I8cdd3d1ade188b7b2278a0eac5510a968229c6fc
joeyparrish added a commit that referenced this issue Jul 13, 2021
When playing multiperiod content, we should not require thumbnails to
be present in every period.  This fixes this case and adds a
regression test.

Closes #3383

Change-Id: I4403a1b8670cffcc46de32470e2d830dc199c9f4
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Sep 10, 2021
@shaka-project shaka-project locked and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

No branches or pull requests

4 participants