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

[Jer Noble]

> On Nov 14, 2017, at 1:36 PM, Alicia Boya García <aboya at igalia.com> wrote:
> 
> 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.

Aha, okay.  There’s also SampleIsLessThanMediaTimeComparator, SampleIsGreaterThanMediaTimeComparator, which does take duration into account, but those look to be unused.

> 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.

I think we should consider the entire interval, though. See below:

> 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.

Yeah, the spec is not the best here.  If you have, e.g. a sample with a PTS of 0 and a duration of 100, and then insert a sample with a PTS of 50 and a duration of 100, you’d expect that to cause the first sample to be removed. But a strict reading of the spec says that sample stays.  Now you have two overlapping samples.  It can get even weirder if you insert a sample with a PTS of 25 and a duration of 50.  Now, strictly implementing the spec, you have a sample overlaps on both ends of another sample.  What does that even mean for a decoder?  It’s almost guaranteed to generate a decode error, unless both of the overlapping samples are I-frames.

I think the intent of the spec is clear: if any part of a previous sample overlaps the new one, it has to be removed, and all samples that depend on the removed samples must be removed as well.  And in order to do that, you have to take the entire duration of the sample into account.

> 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 <https://bugs.webkit.org/show_bug.cgi?id=179690>

Okay, I think we can take this conversation to the bug comment section. :)

-Jer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20171114/7ffdbb47/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3381 bytes
Desc: not available
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20171114/7ffdbb47/attachment.bin>