fix: Treat regions uniquely (shaka-project#4841)

Captions split over stream segment boundaries are repeated over more than one segment. Regions in which they sit maybe described the same but allocated different ids and vice versa. Proposal to treat internal region ids uniqely by encoding the dimensions in the id. fixes shaka-project#4839
Robloche · Jan 27, 2023 · c737006 · c737006
1 parent 2cba86c
commit c737006
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 4 deletions.
diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js
@@ -371,6 +371,25 @@ shaka.text.UITextDisplayer = class {
     }
   }
 
+  /**
+   * Compute a unique internal id:
+   * Regions can reuse the id but have different dimensions, we need to
+   * consider those differences
+   * @param {shaka.extern.CueRegion} region
+   * @private
+   */
+  generateRegionId_(region) {
+    const percentageUnit = shaka.text.CueRegion.units.PERCENTAGE;
+    const heightUnit = region.heightUnits == percentageUnit ? '%' : 'px';
+    const viewportAnchorUnit =
+        region.viewportAnchorUnits == percentageUnit ? '%' : 'px';
+    const uniqueRegionId = `${region.id}_${
+      region.width}x${region.height}${heightUnit}-${
+      region.viewportAnchorX}x${region.viewportAnchorY}${viewportAnchorUnit}`;
+
+    return uniqueRegionId;
+  }
+
   /**
    * Get or create a region element corresponding to the cue region.  These are
    * cached by ID.
@@ -382,8 +401,9 @@ shaka.text.UITextDisplayer = class {
   getRegionElement_(cue) {
     const region = cue.region;
 
-    if (this.regionElements_.has(region.id)) {
-      return this.regionElements_.get(region.id);
+    const regionId = this.generateRegionId_(region);
+    if (this.regionElements_.has(regionId)) {
+      return this.regionElements_.get(regionId);
     }
 
     const regionElement = shaka.util.Dom.createHTMLElement('span');
@@ -394,7 +414,7 @@ shaka.text.UITextDisplayer = class {
     const viewportAnchorUnit =
         region.viewportAnchorUnits == percentageUnit ? '%' : 'px';
 
-    regionElement.id = 'shaka-text-region---' + region.id;
+    regionElement.id = 'shaka-text-region---' + regionId;
     regionElement.classList.add('shaka-text-region');
 
     regionElement.style.height = region.height + heightUnit;
@@ -415,7 +435,7 @@ shaka.text.UITextDisplayer = class {
       regionElement.style.justifyContent = 'flex-end';
     }
 
-    this.regionElements_.set(region.id, regionElement);
+    this.regionElements_.set(regionId, regionElement);
     return regionElement;
   }
 

diff --git a/test/text/ui_text_displayer_unit.js b/test/text/ui_text_displayer_unit.js
@@ -537,4 +537,74 @@ describe('UITextDisplayer', () => {
         '.shaka-text-region');
     expect(allRegionElements.length).toBe(1);
   });
+
+  it('creates separate regions when dimensions differ but id same', () => {
+    const identicalRegionId = 'regionId';
+
+    const cueRegion1 = new shaka.text.CueRegion();
+    const cueRegion2 = new shaka.text.CueRegion();
+    cueRegion1.id = identicalRegionId;
+    cueRegion2.id = identicalRegionId;
+
+    cueRegion1.height = 80;
+    cueRegion1.heightUnits = shaka.text.CueRegion.units.PERCENTAGE;
+    cueRegion1.width = 80;
+    cueRegion1.widthUnits = shaka.text.CueRegion.units.PERCENTAGE;
+
+    cueRegion2.height = 160; // the only difference!
+    cueRegion2.heightUnits = shaka.text.CueRegion.units.PERCENTAGE;
+    cueRegion2.width = 80;
+    cueRegion2.widthUnits = shaka.text.CueRegion.units.PERCENTAGE;
+
+    cueRegion1.viewportAnchorX = 10;
+    cueRegion1.viewportAnchorY = 10;
+    cueRegion1.viewportAnchorUnits = shaka.text.CueRegion.units.PERCENTAGE;
+
+    cueRegion2.viewportAnchorX = 10;
+    cueRegion2.viewportAnchorY = 10;
+    cueRegion2.viewportAnchorUnits = shaka.text.CueRegion.units.PERCENTAGE;
+
+    // These all attach to the same region, but only one region element should
+    // be created.
+    const firstBatchOfCues = [
+      new shaka.text.Cue(0, 100, ''),
+      new shaka.text.Cue(0, 100, ''),
+      new shaka.text.Cue(0, 100, ''),
+    ];
+    for (const cue of firstBatchOfCues) {
+      cue.displayAlign = shaka.text.Cue.displayAlign.CENTER;
+      cue.region = cueRegion1;
+    }
+
+    // Another batch for the other region
+    const secondBatchOfCues = [
+      new shaka.text.Cue(0, 100, ''),
+      new shaka.text.Cue(0, 100, ''),
+      new shaka.text.Cue(0, 100, ''),
+    ];
+    for (const cue of secondBatchOfCues) {
+      cue.displayAlign = shaka.text.Cue.displayAlign.CENTER;
+      cue.region = cueRegion2;
+    }
+
+    textDisplayer.setTextVisibility(true);
+    textDisplayer.append(firstBatchOfCues);
+    textDisplayer.append(secondBatchOfCues);
+    updateCaptions();
+
+    const textContainer = videoContainer.querySelector('.shaka-text-container');
+    const allRegionElements = textContainer.querySelectorAll(
+        '.shaka-text-region');
+
+    // Verify that the nested cues are attached to respective region element.
+    expect(allRegionElements.length).toBe(2);
+
+    const childrenOfOne = Array.from(allRegionElements[0].childNodes).filter(
+        (e) => e.nodeType == Node.ELEMENT_NODE);
+    expect(childrenOfOne.length).toBe(3);
+
+    const childrenOfTwo = Array.from(allRegionElements[1].childNodes).filter(
+        (e) => e.nodeType == Node.ELEMENT_NODE);
+    expect(childrenOfTwo.length).toBe(3);
+  });
 });