[webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's

[Alicia Boya García]

Hi, Jer.

Sorry for the late reply, I've been busy investigating all of this.

I'm replying to your points in reverse order...

On 09/11/17 23:23, Jer Noble wrote:
>> As I understand it, the point of that part of the algorithm is to delete
>> old samples that are -- even partially -- in the presentation time range
>> of the newly appended one, but using (beginTime, endTime] fails to
>> accomplish that in two cases:
>>
>> a) If there is an existing MediaSample with PTS=9 and DUR=1 it will not
>> be removed because beginTime (=9) is exclusive.
>
> Eh, not really. The search space is the entire duration of the sample,
> so an exclusive beginTime of 9 _should_ match PTS=9, DUR=1 because 9 <
> presentationEndTime=10.  I’m pretty sure this is correct, but a mock
> test case should clear this up pretty quickly.

That's totally not the case, though the first time I gave a look at this
code I thought that too... presentationRange() is a std::map whose
key type is a single MediaTime specifying the PTS of the MediaSample,
not the duration or the full presentation time range.

Also, neither findSamplesWithinPresentationRange() or its siblings take
durations into account, as they search only with the key (PTS) in
mind... Well, actually findSamplesWithinPresentationRangeFromEnd() looks
at the values, but it only checks .presentationTimestamp() so it's
functionally the same.

Adding a sample with same PTS as an existing one does not remove the old
one with this check. There are two possible behaviors that can mask the
issue:

a) The previous frame removal step (see step 1.13 in the spec or 1.14 in
WebKit) removed the overlapping frame. This is only the case if the
frame started a new Coded Frame Group.

b) The existing overlapping frame was not removed, but later on when the
new MediaSample was passed to std::map::insert() it was ignored because
it was a duplicated key (same PTS as the existing overlapping frame).

You can confirm that it's the old MediaSample the one persisted by
setting different durations for the old and the new one but the same
PTS. After the append of the second frame, the old duration persists and
there are no new frames in the TrackBuffer.

On 09/11/17 23:23, Jer Noble wrote:
>> My question is... shouldn't the range ends inclusivity be the other way
>> around i.e. [beginTime, endTime)?
> 
> beginTime was intended to be exclusive so that, given a [0, 9] range, searching for (9, 10] wouldn’t match the last sample in the [0, 9] range.
> 
> But now that you mention it, perhaps the real problem is that both beginTime _and_ endTime should be exclusive.

This is not an issue because as explained before we are considering just
the frame start PTS, not its entire presentation interval.

beginTime must be inclusive so that old frames with the same PTS are
deleted. endTime must be exclusive so that non overlapping consecutive
frames are not deleted.

I have checked this with the spec:
https://www.w3.org/TR/media-source/#sourcebuffer-coded-frame-processing

1.14. Remove existing coded frames in track buffer:
 -> If highest end timestamp for track buffer is not set:
       [...]
 -> If highest end timestamp for track buffer is set and less than or
    equal to presentation timestamp:
       Remove all coded frames from track buffer that have a
       presentation timestamp greater than or equal to highest end
       timestamp and less than frame end timestamp.


On 09/11/17 23:23, Jer Noble wrote:
> It should be possible to make a “mock” testcase which doesn’t rely on
> any particular media engine and which can demonstrate this bug.

I just finished doing that. I have written two new MediaSource tests
using video/mock: one that demonstrates how existing frames whose PTS
coincide with the start of the range of the range are not deleted and
another one that demonstrates how existing frames whose PTS coincide
with the end of the range are deleted.

I have also written a patch with the correct checks:
https://bugs.webkit.org/show_bug.cgi?id=179690

-- Alicia.