Bug 180258 – [MSE] Add isValid() check before using trackBuffer.lastEnqueuedPresentationTime

• Component: Media
• Status: RESOLVED FIXED

[Alicia Boya García]

The current comparison in sourceBufferPrivateDidReceiveSample() does not take in account the possibility of lastEnqueuedPresentationTime not being set.

MediaTime currentMediaTime = m_source->currentTime();
if (currentMediaTime < trackBuffer.lastEnqueuedPresentationTime) {
    PlatformTimeRanges possiblyEnqueuedRanges(currentMediaTime, trackBuffer.lastEnqueuedPresentationTime);
    possiblyEnqueuedRanges.intersectWith(erasedRanges);
    if (possiblyEnqueuedRanges.length())
        trackBuffer.needsReenqueueing = true;
}

Is this intentional? When experimenting with a new frame removal algorithm I hit a case where it was unset and therefore it created an invalid range in possiblyEnqueuedRanges.

[Alicia Boya García]

Created attachment 328114 [details]
Patch

[Jer Noble]

Comment on attachment 328114 [details]
Patch

Is this change fix something, and if so, should it have a test?

[Alicia Boya García]

No.(In reply to Jer Noble from comment #2)
> Comment on attachment 328114 [details]
> Patch
> 
> Is this change fix something, and if so, should it have a test?

I'm not sure, that's why I'm asking if the assumption in the `if` above was intentional.

As I see it, if is not intentional (i.e. it's possible that lastEnqueuedPresentationTime is MediaTime::invalidTime()) then we should check for that as in the patch.

If it's intentional (i.e. it can never be invalid time), it's not obvious and should have an ASSERT() to make it more evident and avoid unexpected side effects should that ever cease being the case.

[Alicia Boya García]

Could we take a decision on this?

[WebKit Commit Bot]

Comment on attachment 328114 [details]
Patch

Clearing flags on attachment: 328114

Committed r225936: <https://trac.webkit.org/changeset/225936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36059745>