Skip to content

Commit

Permalink
fix: Fix filtering of duplicate image streams
Browse files Browse the repository at this point in the history 
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 shaka-project#3383

Change-Id: I63355f85d4e12bd25c1cadce29a040d17c4e7d61
  • Loading branch information
@joeyparrish
joeyparrish committed Jul 12, 2021
1 parent 052e794 commit 42674be
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 0 deletions.
31 changes: 31 additions & 0 deletions lib/util/periods.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ shaka.util.PeriodCombiner = class {
shaka.util.PeriodCombiner.filterOutAudioStreamDuplicates_(periods);
shaka.util.PeriodCombiner.filterOutVideoStreamDuplicates_(periods);
shaka.util.PeriodCombiner.filterOutTextStreamDuplicates_(periods);
shaka.util.PeriodCombiner.filterOutImageStreamDuplicates_(periods);

// Optimization: for single-period VOD, do nothing. This makes sure
// single-period DASH content will be 100% accurately represented in the
Expand Down Expand Up @@ -342,6 +343,36 @@ shaka.util.PeriodCombiner = class {
}
}

/**
* @param {!Array.<shaka.util.PeriodCombiner.Period>} periods
* @private
*/
static filterOutImageStreamDuplicates_(periods) {
// Two image streams are considered to be duplicates of
// one another if their ids are different, but all the other
// information is the same.
for (const period of periods) {
const filteredImages = [];
for (const i1 of period.imageStreams) {
let duplicate = false;
for (const i2 of filteredImages) {
if (i1.id != i2.id &&
i1.width == i2.width &&
i1.codecs == i2.codecs &&
i1.mimeType == i2.mimeType) {
duplicate = true;
}
}

if (!duplicate) {
filteredImages.push(i1);
}
}

period.imageStreams = filteredImages;
}
}

/**
* Stitch together DB streams across periods, taking a mix of stream types.
* The offline database does not separate these by type.
Expand Down
62 changes: 62 additions & 0 deletions test/util/periods_unit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 2),
],
textStreams: [],
imageStreams: [],
},
{
id: 'ad',
Expand All @@ -49,6 +50,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 2),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -117,6 +119,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 2),
],
textStreams: [],
imageStreams: [],
},
{
id: 'main',
Expand All @@ -130,6 +133,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 2),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -194,6 +198,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -204,6 +209,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -242,6 +248,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -252,6 +259,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -289,6 +297,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('fr', /* channels= */ 2, /* primary= */ false),
],
textStreams: [],
imageStreams: [],
},
{
id: 'ad',
Expand All @@ -299,6 +308,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
{
id: 'show2',
Expand All @@ -311,6 +321,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('fr', /* channels= */ 2, /* primary= */ true),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -361,6 +372,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('es'),
],
textStreams: [],
imageStreams: [],
},
{
id: 'show2',
Expand All @@ -371,6 +383,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -411,6 +424,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en'),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -495,6 +509,16 @@ describe('PeriodCombiner', () => {
t3.originalId = 't3';
t3.roles = ['role1'];

// i1 and i3 are duplicates.
const i1 = makeImageStream(240);
i1.originalId = 'i1';

const i2 = makeImageStream(480);
i2.originalId = 'i2';

const i3 = makeImageStream(240);
i3.originalId = 'i3';

/** @type {!Array.<shaka.util.PeriodCombiner.Period>} */
const periods = [
{
Expand All @@ -516,6 +540,11 @@ describe('PeriodCombiner', () => {
t2,
t3,
],
imageStreams: [
i1,
i2,
i3,
],
},
];

Expand Down Expand Up @@ -543,6 +572,15 @@ describe('PeriodCombiner', () => {
for (const id of textIds) {
expect(id).not.toBe('t3');
}

const imageStreams = combiner.getImageStreams();
expect(imageStreams.length).toBe(2);

// i3 should've been filtered out
const imageIds = imageStreams.map((i) => i.originalId);
for (const id of imageIds) {
expect(id).not.toBe('i3');
}
});

// Regression test for #3383, where we failed on multi-period content with
Expand Down Expand Up @@ -602,6 +640,7 @@ describe('PeriodCombiner', () => {
textStreams: [
makeTextStream('en'),
],
imageStreams: [],
},
{
id: '2',
Expand All @@ -614,6 +653,7 @@ describe('PeriodCombiner', () => {
textStreams: [
/* No text streams */
],
imageStreams: [],
},
{
id: '3',
Expand All @@ -627,6 +667,7 @@ describe('PeriodCombiner', () => {
makeTextStream('en'),
makeTextStream('es'),
],
imageStreams: [],
},
];

Expand Down Expand Up @@ -662,6 +703,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 6),
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -672,6 +714,7 @@ describe('PeriodCombiner', () => {
makeAudioStream('en', /* channels= */ 2),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -708,6 +751,7 @@ describe('PeriodCombiner', () => {
makeAudioStreamWithSampleRate(48000),
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -718,6 +762,7 @@ describe('PeriodCombiner', () => {
makeAudioStreamWithSampleRate(44100),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -754,6 +799,7 @@ describe('PeriodCombiner', () => {
makeAudioStreamWithSampleRate(44100),
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -764,6 +810,7 @@ describe('PeriodCombiner', () => {
makeAudioStreamWithSampleRate(48000),
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -794,6 +841,7 @@ describe('PeriodCombiner', () => {
],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -803,6 +851,7 @@ describe('PeriodCombiner', () => {
],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -847,6 +896,7 @@ describe('PeriodCombiner', () => {
stream2,
],
textStreams: [],
imageStreams: [],
},
{
id: '1',
Expand All @@ -858,6 +908,7 @@ describe('PeriodCombiner', () => {
stream4,
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -906,6 +957,7 @@ describe('PeriodCombiner', () => {
stream2,
],
textStreams: [],
imageStreams: [],
},
{
id: '2',
Expand All @@ -917,6 +969,7 @@ describe('PeriodCombiner', () => {
stream4,
],
textStreams: [],
imageStreams: [],
},
];

Expand Down Expand Up @@ -1041,52 +1094,60 @@ describe('PeriodCombiner', () => {
videoStreams: [v1, v2, v3, v4, v5],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '2',
videoStreams: [v6, v7, v8, v9, v10],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '3',
videoStreams: [v11, v12, v13, v14, v15],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '4',
videoStreams: [v16, v17, v18, v19, v20],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '5',
// Same as 1st
videoStreams: [v1, v2, v3, v4, v5],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '6',
// Same as 2nd
videoStreams: [v6, v7, v8, v9, v10],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '7',
// Same as 3rd
videoStreams: [v11, v12, v13, v14, v15],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
{
id: '8',
// Same as 4th
videoStreams: [v16, v17, v18, v19, v20],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
// Adding the 1st period again since it was the one that used to
// cause trouble when repeated.
Expand All @@ -1096,6 +1157,7 @@ describe('PeriodCombiner', () => {
videoStreams: [v1, v2, v3, v4, v5],
audioStreams: [],
textStreams: [],
imageStreams: [],
},
];

Expand Down

0 comments on commit 42674be

Please sign in to comment.