Fix: exclude text processors from Stream.getProcessors when text is disabled #3342
Conversation
- removes potential for ambiguity between liveDelay variable in startPlaybackCatchUp function and PlaybackController module when reasoning about the code - new name intended to convey that the player's liveDelay value is used throughout this function
This fixes a bug observed with low-latency streams containing one or more text representations. The bug prevented the liveCatchUpPlaybackRate from being correctly applied following a buffer stall, leading to unconstrained drift from the user-specified target latency. The bug manifested as follows: 1. a buffer stall event occurs, the 'playbackStalled' field on StreamController is set to 'true' 2. 'StreamController.startPlaybackCatchUp' is called, it uses 'StreamController.getBufferLevel' to obtain the lowest current audio, video text or fragmented text buffer level 3. in streams with a text or fragmented text representation, the recent change to disable text by default resulted in an empty buffer being maintained for that representation _unless_ text had been explicitly enabled for the player 4. the empty buffer prevents 'StreamController.startPlaybackCatchUp' from resetting the 'playbackStalled' field of 'StreamController' to 'false', further, it assumes a buffer stall is ongoing, and resets the 'newRate' to '1.0', preventing latency from being reduced This change then checks whether text has been enabled and only adds the stream processors for textual representations to those returned by the 'Stream.getProcessors' if it is. Tested on Firefox 78.02 & Chrome 84.0.414.79 on Ubuntu 20.04.
- existing test verifies that empty array of stream processors exists when stream is not activated - additional tests check that text and fragmented text processors are returned by Stream.getProcessors _only_ when text is enabled via the TextController
03k64
force-pushed
the
bbc-internship-base
branch
from
34c5455
to
1cb20c6
Compare
This fix ensure the correct minimum buffer level is reported when text is disabled. Previously a buffer level of 0 is reported at all times if a text track is present in the manifest but text is disabled in the player. This fix checks whether text is enabled and only includes text buffer metrics in buffer level reporting if it is.
| @@ -714,6 +714,8 @@ function Stream(config) { | |||
| } | |||
|
|
|||
| function getProcessors() { | |||
There was a problem hiding this comment.
getProcessors is used in other functions as well. I would prefer to add a flag to this function "includeTextIfDisabled" or something similar. This flag should default to true and should only be false for the use case you describe.
|
@03k64 Sorry for the delay in reviewing this. I had one comment, see above. Can you also rebase against the current development branch please? Then we can add this to 3.2.2. Thank you! |
|
@03k64 Can you give me short feedback if you are still interested in contributing this? Otherwise I might pick this up and rebase it against the current dev branch. |
Hi, sorry, things have been kind of hectic for the last while - I made the original PR while working on an internship last summer at the BBC. I won't be able to commit the time for the next few weeks (probably not until the second half of June) so if you'd like to work on it before then then please do :) otherwise I will be happy to make the changes you suggested at that point 👍 |
This fixes a bug observed with low-latency streams containing one or more text representations. The bug prevented the
liveCatchUpPlaybackRatefrom being correctly applied following a buffer stall, leading to unconstrained drift from the user-specified target latency.The bug manifested as follows:
playbackStalledfield onStreamControlleris set totrueStreamController.startPlaybackCatchUpis called, it usesStreamController.getBufferLevelto obtain the lowest current audio, video text or fragmented text buffer levelStreamController.startPlaybackCatchUpfrom resetting theplaybackStalledfield ofStreamControllertofalse, further, it assumes a buffer stall is ongoing, and resets thenewRateto1.0, preventing latency from being reducedThis change then checks whether text has been enabled and only adds the stream processors for textual representations to those returned by the
Stream.getProcessorsif it is. I have added some tests to document/verify the new intended behaviour. I have also included a smaller change, that modifies a function-level variable name to provide clarity due to a class-level variable with the same name.This is my first PR, however I am submitting as part of my work at the BBC, who have previously signed up to the dash.js feedback agreement.