Dev/fix pdt search #1601
Dev/fix pdt search #1601
Conversation
Latest public hls.js
Merge to latest
…seconds as they would be in real case scenario in order to properly test the feature
… still return the next segment Fixed an error with the unit tests that made me think that BufferEnd was milliseconds when it is actually seconds MediaSeek should set fragPrevious to null in order not to confuse the algorithm
There was a problem hiding this comment.
Hi @capagrisdesu thanks for you PR, IMHO there are a few items that would require clarification. please check
src/controller/stream-controller.js
Outdated
| @@ -813,6 +818,8 @@ class StreamController extends TaskLoop { | |||
| if (!this.loadedmetadata) | |||
| this.nextLoadPosition = this.startPosition = currentTime; | |||
|
|
|||
| this.fragPrevious = null; | |||
There was a problem hiding this comment.
there are several conditions in onMediaSeeking()that already nullify fragPrevious when needed.
for example if currentTime = 10s, media is buffered from 0 to 100s, and you seek to t=20s, there is no need to nullify fragPrevious, as this seek will be completely transparent (in the sense that there is no need to disturb frag loading/parsing/appending)
nullifying fragPrevious on seeking is only useful if we detect that we are seeking to a position that requires a change in frag loading (ie next frag loading will not be contiguous with the previous one)
There was a problem hiding this comment.
I see. When depending on PDT from the fragment to fetch the next one, then it is important than the next one is contiguous. That is why I thought of nullifying it. Then, I should change the implementation to depend on bufferEnd. That is another option that I believe it would work (in my tests seem to work).
| return BinarySearch.search(fragments, function (frag) { | ||
| return PDTValue < frag.pdt ? -1 : PDTValue >= frag.endPdt ? 1 : 0; | ||
| }); | ||
| for(let seg = 0; seg < fragments.length; ++seg) |
There was a problem hiding this comment.
if PDT is monotonic, then binary search should be equivalent and more efficient, I am not clear about the issue you are trying to address here ?
There was a problem hiding this comment.
It does have order but there are gaps. That is the main difference with a sequential one in which the start parameter is set contiguous independently of discontinuities. Not for PDT. If between two segments there is a discontinuity and you request a jump in between with a binary search is unable to find the segment. This is checked with a unit test.
|
Since this fixes a regression introduced by a PR in master I'm tagging this for the next release |
|
makes sense to target next release. i understand the fix for |
…contiguity after a media seek.
|
I have submitted my last commit as an alternative. This way is not nullifying the previous segment at media seek which is consistent which your design. On the other hand, PDT will depend more on the bufferEnd which should be the same as if there is a vacuum due to a discontinuity the new for (instead of the binary search) will select the next one. If you prefer the original solution, I can revert. Please ask any questions about it |
| assert.equal(foundFragment, fragments[2], 'Expected sn 2, found sn segment ' + resultSN); | ||
| }); | ||
|
|
||
| it('PDT serch hitting empty discontinuity', function () { |
There was a problem hiding this comment.
If you replace the former binary search, this unit test will fail. If a user selects a time that actually is a discontinuity, because the binary search only checks for if the PDT is within the segment. I could not figure out how to use the binary search taking into account next and former segment. I could also let the binary search stay as it is more efficient, and if null, then do the for. Is this preferable?
There was a problem hiding this comment.
fair enough. as _findFragmentByPDT() is not expected to be triggered everytime, it should be fine
|
What is PDT? That acronym is not present in ISO/IEC 13818-1 |
|
@jon-valliere - program date time |
Description of the Changes
Testing some streams showed me that media seeking failed to perform properly when PDT is being used. I realized that the issue is that fragPrevious is not nullified. I also realized that a few more changes were actually needed.
Description of changes:
I am very curious into why fragPrevious is not nullified when seeking is requested. Former segment is of no relevance or confusing as there is no connection between the jump and the current position. Please someone can explain? Maybe that is required for some obscure reason I can't see and this should not be committed as it is.
CheckLists