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.

Sign up for GitHub

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

Jump to bottom

fix(DASH): Ensure variants are created for unique video codec bases #6835

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+94 −2
Open

fix(DASH): Ensure variants are created for unique video codec bases #6835

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d085a3f
Keep matched stream of diff codec tag unused
willdharris Jun 12, 2024
48cde01
Revert variable move
willdharris Jun 13, 2024
b2dc800
Update stream match test
willdharris Jun 14, 2024
af10bae
Update comment
willdharris Jun 14, 2024
b870d2d
Update email
willdharris Jun 15, 2024
7727f7f
Create stream per unique audio codec
willdharris Jun 24, 2024
fa0f119
Add audio stream per codec test
willdharris Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions lib/util/periods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ shaka.util.PeriodCombiner = class {
periodsMissing) {
const ContentType = shaka.util.ManifestParserUtils.ContentType;
const LanguageUtils = shaka.util.LanguageUtils;
const MimeUtils = shaka.util.MimeUtils;
const matches = outputStream.matchedStreams;

// Assure the compiler that matches didn't become null during the async
Expand All @@ -734,6 +735,24 @@ shaka.util.PeriodCombiner = class {
used = false;
}
}
// In cases where we have the same video codec but different
// codec bases in matched representations across periods
// (e.g. HEVC as hev and hvc), we don't want to delete the matched stream
Copy link
Member

Choose a reason for hiding this comment

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

I might be misunderstanding, but it seems like the solution could be just to normalize video codecs in streams before processing periods and in configs before applying preferences. If they all said hev or all said hvc, problem solved, correct?

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 could work, but I think would still result in confusion in some cases, like we are finding with manifests returned from Google DAI.

For example, we have HEVC content encoded in a hvc format. When requesting a stitched manifest from DAI, with DAI pre-rolls and our original content, the HEVC pre-roll representations from DAI are encoding in a hev format. As a content owner, I know there are hvc representations in my manifest but may not know DAI has used a different format for pre-rolls. If Shaka Player "converts" hvc to hev, then when no hvc variants exist it's confusing as to why those aren't available.

Outside the scope of this change but related: In the same manifest we also have content encoded as dvhe. DAI does not encode ads in DVHE. Shaka Player matches the content period dvhe streams with the pre-roll hev streams. dvhe variants are still created even though DVHE streams aren't available in pre-rolls. For conformity with that behavior, I think it makes sense to create unique variants for each codec base.

@dsparacio Any additional thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm going on leave, so @theodab and @avelad should make a decision about this without waiting for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we go in the direction @joeyparrish suggested? I guess it would help in majority of cases. I'm afraid with the proposed solution we might end up with plenty of almost-duplicated variants and it can be confusing for devs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree! @theodab what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thoughts on this.

My main concern is the difference with HVC and DVHE when mixed with HEV from DAI content.

  1. As Will pointed out, currently, if we set preferredVideoCodec to [hev, hvc] we get a bitrate ladder with 14 reps instead of 7 and need to filter, but all are marked as HEV even though some are HVC.

  2. When I use preferredVideoCodec[hev, dvhe] I get same as aboven and it excludes DVHE altogether.

  3. When I use preferredVideoCodec[dvhe] I get exactly what I am expecting, 7 bitrates all DVHE representations.

So as a user, I would expect that using preferredVideoCodec[hvc] would result the same as preferredVideoCodec[dvhe]. However it only returns 3 items due to the issue we pointed out in the ticket.

It does seem it would be more efficient to do this the way Joey suggested. I just want to make sure we have some consistency in the properties settings and expected results.

Meaning either works for us.... but its odd if they are different IMHO

 preferredVideoCodec to [hev, dvhe] - does not return any DVHE (same result as hev, hvc)
 preferredVideoCodec to [hev, hvc] - this returns twice as many bitrates as expected since DAI does not exactly match at resolution.

Or

 preferredVideoCodec to [dvhe] works as expected today
 preferredVideoCodec to [hvc] - truncated list returned.

Currently api settings

 preferredVideoCodec to [dvhe] 
 preferredVideoCodec to [hev, hvc] - have to filter this on our side.

// from unusedStreamsPerPeriod until an outputStream for each
// video representation has been created.
if (outputStream.type == ContentType.VIDEO) {
avelad marked this conversation as resolved.
Show resolved Hide resolved
const outputStreamNormalizedCodec =
MimeUtils.getNormalizedCodec(outputStream.codecs);
const matchNormalizedCodec = MimeUtils.getNormalizedCodec(match.codecs);
const outputStreamCodecBase =
MimeUtils.getCodecBase(outputStream.codecs);
const matchCodecBase = MimeUtils.getCodecBase(match.codecs);

if (outputStreamNormalizedCodec == matchNormalizedCodec &&
outputStreamCodecBase != matchCodecBase) {
used = false;
}
}

if (used) {
unusedStreamsPerPeriod[i - periodsMissing].delete(match);
Expand Down
21 changes: 19 additions & 2 deletions test/util/periods_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,8 @@ describe('PeriodCombiner', () => {
expect(audio2.originalId).toBe('2,4');
});

it('Matches streams with related codecs', async () => {
it('Creates a video stream for each unique codec base ' +
'and matches streams with related codecs', async () => {
const stream1 = makeVideoStream(1080);
stream1.originalId = '1';
stream1.bandwidth = 120000;
Expand Down Expand Up @@ -1189,19 +1190,35 @@ describe('PeriodCombiner', () => {

await combiner.combinePeriods(periods, /* isDynamic= */ true);
const variants = combiner.getVariants();
expect(variants.length).toBe(4);
expect(variants.length).toBe(7);
// We can use the originalId field to see what each track is composed of.
const video1 = variants[0].video;
expect(video1.originalId).toBe('1,2');
expect(video1.codecs).toBe(stream1.codecs);

const video2 = variants[1].video;
expect(video2.originalId).toBe('3,4');
expect(video2.codecs).toBe(stream3.codecs);

const video3 = variants[2].video;
expect(video3.originalId).toBe('5,6');
expect(video3.codecs).toBe(stream5.codecs);

const video4 = variants[3].video;
expect(video4.originalId).toBe('7,8');
expect(video4.codecs).toBe(stream7.codecs);

const video5 = variants[4].video;
expect(video5.originalId).toBe('1,2');
expect(video5.codecs).toBe(stream2.codecs);

const video6 = variants[5].video;
expect(video6.originalId).toBe('3,4');
expect(video6.codecs).toBe(stream4.codecs);

const video7 = variants[6].video;
expect(video7.originalId).toBe('5,6');
expect(video7.codecs).toBe(stream6.codecs);
});

it('Matches streams with most roles in common', async () => {
Expand Down
Loading