Hi Alicia, It should be possible to make a “mock” testcase which doesn’t rely on any particular media engine and which can demonstrate this bug.
Continued:
> On Nov 9, 2017, at 2:03 PM, Alicia Boya García <ab...@igalia.com> wrote:
>
> Hi, WebKittens!
>
> In the YouTube Media Source Extensions conformance tests there is one
> called 36.AppendOpusAudioOutOfOrder where two audio media segments are
> appended out of order to a SourceBuffer: First, a segment with the PTS
> ranges [10, 20) is added. Then, another one with [0, 10) is added.
>
> (I have rounded the actual timestamps to near integers for easier
> understanding).
>
> Almost at the very end of the process the buffered ranges are like this:
>
> [ 0, 9)
> [10, 20)
>
> At this point, SourceBuffer::sourceBufferPrivateDidReceiveSample() is
> called with the last audio frame, that has PTS=9 and DUR=1.
>
> The execution reaches this conditional block:
>
> ```
> if (trackBuffer.highestPresentationTimestamp.isValid() &&
> trackBuffer.highestPresentationTimestamp <= presentationTimestamp) {
> ```
>
> trackBuffer.highestPresentationTimestamp contains the highest PTS so far
> within the current segment. The condition is true (9 <= 9) as expected
> for sequentially appended frames.
>
> Inside there is this block of code:
>
> ```
> MediaTime highestBufferedTime = trackBuffer.buffered.maximumBufferedTime();
>
> PresentationOrderSampleMap::iterator_range range;
> if (highestBufferedTime - trackBuffer.highestPresentationTimestamp <
> trackBuffer.lastFrameDuration)
> range =
> trackBuffer.samples.presentationOrder().findSamplesWithinPresentationRangeFromEnd(trackBuffer.highestPresentationTimestamp,
> frameEndTimestamp);
> else
> range =
> trackBuffer.samples.presentationOrder().findSamplesWithinPresentationRange(trackBuffer.highestPresentationTimestamp,
> frameEndTimestamp);
>
> if (range.first != trackBuffer.samples.presentationOrder().end())
> erasedSamples.addRange(range.first, range.second);
> ```
>
> The first if block there is an optimization, it decides whether to do a
> binary search in the entire collection of MediaSample's or do a linear
> search starting with the MediaSample with the highest PTS (which is
> faster when appends always occur at the end), but the result is the same
> in both cases.
>
> presentationOrder() is a std::map<MediaTime, MediaSample>.
>
> findSamplesWithinPresentationRange(beginTime, endTime) and its *FromEnd
> counterpart both return a pair of STL-style iterators which cover a
> range of MediaSample objects whose presentation timestamps sit in the
> range (beginTime, endTime] (beginTime is exclusive, endTime is inclusive).
>
> Then, it marks those MediaSample objects (frames) for deletion.
>
> 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.
> 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.
> b) If there is an existing MediaSample with PTS=10 and DUR=1 it WILL be
> removed even though there is no overlap with the sample being appended
> (PTS=9 DUR=1) because endTime (=10) is inclusive. This is exactly what
> is making the YTTV test fail in my case.
This may be true because endTime is inclusive (and shouldn’t be).
> Before:
>
> [ 0, 9)
> [10, 20)
>
> Expected result after adding [9, 10):
>
> [0, 20)
>
> Actual result in WebKit:
>
> [ 0, 10)
> [11, 20)
It looks like findSamplesWithinPresentationRange() is only ever used in that
specific part of sourceBufferPrivateDidReceiveSample(), so this should be very
safe to change.
-Jer
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
