Bug 139813 – [MSE] Implement per TrackBuffer buffered.

• Component: Media
• Status: REOPENED

[Bartlomiej Gajda]

Section 3.1 of MSE spec, regarding buffered() clearly indicates that it should be per trackBuffer, not per SourceBuffer.

[Bartlomiej Gajda]

Created attachment 243548 [details]
patch_wip_per_trackbuffer_v1

In this version I added only per trackBuffer TimeRanges, and implemented algorithm from "3.1 buffered, get" as caching mechanism.

This however is not sufficient, for part ragarding "ended" state.

There are 2 options I think:

- keep caching, reverse order of operations from algorithm (so : get intersection, get highest time from trackBuffers, append till end if neccessary), but this is almost as much work as not caching at all

- drop the caching and just use this algorithm every time

I'm not yet convinced which approach is better - feel free to comment, I will think over weekend and upload better version next week.

[Bartlomiej Gajda]

In the "reversed order" of course the first point (get intersection) would be cached value.

[Jer Noble]

Comment on attachment 243548 [details]
patch_wip_per_trackbuffer_v1

View in context: https://bugs.webkit.org/attachment.cgi?id=243548&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:195
> +        auto& ranges = trackBuffer.m_buffered->ranges();
> +        unsigned length = ranges.length();
> +        if (length)
> +            highestEndTime = std::max(highestEndTime, ranges.end(length - 1));
> +    }

PlatformTimeRanges has a maximumBufferedTime() which would work well here.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:213
> +        if (ended && trackRanges.length())
> +            trackRanges.add(trackRanges.start(trackRanges.length() - 1), highestEndTime);

Again, maximumBufferedTime() could be used here.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1553
> +    cacheBuffered();

recalculateBuffered()?

[Bartlomiej Gajda]

> PlatformTimeRanges has a maximumBufferedTime() which would work well here.
> Again, maximumBufferedTime() could be used here.

will fix next week.

> recalculateBuffered()?

Good name, but ultimately I think I will drop caching.

I thought it's safe to cache intersection, and at the moment of calling buffered() just : if (ended) cachedBuffered.add(cachedBuffered.end, highestEndTime), but this does not work for:

1st trackBuffer buffered: (0,2) (7,10)
2nd trackBuffer buffered: (4-6)

"Correct" answer from spec would be (7-10) as both trackBuffers would be extended to 10th second by point 4.2 of buffered.

My approach would have cached buffered intersection = (0,0), so it would add (0-10).

So the alternative would be to remember "did MediaSource ended state changed", and then do recalculate buffered but that's duplicating code and I don't see any improvment by that appraoch..

I think I will just call this algorithm directly each time, but that will be done next week, along with more layoutTest for 'ended' scenarios.

[Bartlomiej Gajda]

Created attachment 244596 [details]
add per trackBuffer buffered + layoutTests

So I've returned to my original concept with caching because all the hasCurrentTime()/hasFutureTime() already makes copies of buffered, I didn't want to put even more overhead for each of those calls.

As for handling readyState change, I just added recalculateBuffered() there, so there are 3 points of recaching now: adding samples, removing samples, changing ready state.

I have added tests for simple 2 tracks appends, and for MediaSource.readyState changes.

[Jer Noble]

Comment on attachment 244596 [details]
add per trackBuffer buffered + layoutTests

View in context: https://bugs.webkit.org/attachment.cgi?id=244596&action=review

This is great. What I'd like to see added, but maybe in another patch, is caching of buffered ranges at the MediaSource level. Right now, an expensive merge step happens every time someone requests buffered().

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:821
> +    recalculateBuffered();

The way we do this in other places, like HTMLMediaElement, is that instead of recalculating the cached value immediately when its invalidated, we mark it as dirty and only recalculate the value when asked.  This would mean replacing all your instances of "recalculateBuffered()" to something like "invalidateBuffered()", and call "recalculateBuffered()" from within "buffered()" if the value is invalidated.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1615
> +    recalculateBuffered();

This is probably more often than necessary. You could call this (or invalidate buffered) from sourceBufferPrivateAppendComplete().

[Bartlomiej Gajda]

> This is great. What I'd like to see added, but maybe in another patch, is
> caching of buffered ranges at the MediaSource level. Right now, an expensive
> merge step happens every time someone requests buffered().

I will add caching for MediaSource after this, shouldn't be too hard, but don't want mix stuff too much. (would need to also handle activeSourceBuffers there)
 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:821
> > +    recalculateBuffered();
> 
> The way we do this in other places, like HTMLMediaElement, is that instead
> of recalculating the cached value immediately when its invalidated, we mark
> it as dirty and only recalculate the value when asked.  This would mean
> replacing all your instances of "recalculateBuffered()" to something like
> "invalidateBuffered()", and call "recalculateBuffered()" from within
> "buffered()" if the value is invalidated.

Done.
 
> This is probably more often than necessary. You could call this (or
> invalidate buffered) from sourceBufferPrivateAppendComplete().

Didn't thought about that, You're right, it would be way better. Will fix.

[Bartlomiej Gajda]

Created attachment 244617 [details]
add per trackBuffer buffered + layoutTests v2

Changed code according to comments.

Switched buffered (and new flag for dirtiness as well) to mutable, as it's easier then removing const from 5 functions.

Since I switched to lazy evaluation and there were few places where 'SourceBuffer.m_buffered' was used directly, had to change them as well, to guarantee proper sync with data.

[Jer Noble]

Comment on attachment 244617 [details]
add per trackBuffer buffered + layoutTests v2

The only nit I have is that 'm_shouldRecalculateBuffered = true' could be replaced by 'm_buffered = nullptr', but it's not a blocker.

[WebKit Commit Bot]

Comment on attachment 244617 [details]
add per trackBuffer buffered + layoutTests v2

Clearing flags on attachment: 244617

Committed r178438: <http://trac.webkit.org/changeset/178438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

It looks like this change consistently causes a LayoutTest failure on the bots:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK1%20%28Tests%29/builds/1530

    http/tests/media/media-source/mediasource-remove.html

Should we skip the test? Roll out the change?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140484

[Bartlomiej Gajda]

Failing test in this : https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20%28Tests%29/r178455%20%281534%29/http/tests/media/media-source/mediasource-remove-actual.txt
consist of 2 parts : mismatch of buffered, and DOMException.

Since I don't have any Mac around, I analyzed that file used to append: LayoutTests/http/tests/media/media-source/mp4/test.mp4

This file consists of 2 tracks:
track 1: audio, 260 samples * 1024 duration / 44100 timescale = 6,037s
track 2: video, 182 samples * 83 duration / 2500 timescale = 6.0424s

and there is initial duration from (moov.mvex.mehd duration 3622) / (moov.mvhd timescale 600) = 6,03666.


* Mismatch buffered() part

It's the exact expected behavior after this patch, to use common intersection of ranges, which is 6.037, not 6.0424, so I believe that either this patch should should change `mediasource-util.js` "duration: 6.0424" to "duration: 6.037", or skip test and someone with Mac access would fix testcase later?
I don't know how other tests will behave, my own mp4 parser doesn't handle trackId's yet.

* Dom exception part

I'm not sure yet, but I suspect that this is because track2 extended duration to 6.042, and because of current "// FIXME: Re-number or update this section once" in streamEndedWithError() which reorders readystate change, setDurationInternal() uses buffered which was *not* extended to 'highest end time' (so 6.037, not 6.042) => calls setDurationinternal() with different old and new durations => sets updating to true => "sourceBuffer.remove(1, 2);" has InvalidStateError, as it should wait for 'updateend'.

This can be fixed by doing patch for that FIXME I mentioned (since that bug in spec already got resolved), but there could be perhaps check for is it safe to call remove?

--

Summing up, I'm not sure what should I do with this patch next. As for Jer Noble comment about nullifying buffered, instead of using additional flag - I'd like to change this in next patch with caching buffered for MediaSource. 

As I said I don't have Mac to properly change all those constants in test (like 1.029, 3.187, etc.) which probably will be off after this patch (so only changing segment duration to shorter version won't be enough).


As a side note: I wonder shouldn't this be cleared in standard, because at the moment, when removal occurs, 'sourceended' being posted before 'updateend' seems wrong . I'd expect at 'sourceended' that if there was any removal it is done already, and updating is false, otherwise every user would have to add additional check before taking any action, for 'updating' which might or might not occur. I will probably ask this on W3 bugzila.

[Jer Noble]

(In reply to comment #14)
> Failing test in this :
> https://build.webkit.org/results/
> Apple%20Yosemite%20Debug%20WK1%20%28Tests%29/r178455%20%281534%29/http/tests/
> media/media-source/mediasource-remove-actual.txt
> consist of 2 parts : mismatch of buffered, and DOMException.
> 
> Since I don't have any Mac around, I analyzed that file used to append:
> LayoutTests/http/tests/media/media-source/mp4/test.mp4
> 
> This file consists of 2 tracks:
> track 1: audio, 260 samples * 1024 duration / 44100 timescale = 6,037s
> track 2: video, 182 samples * 83 duration / 2500 timescale = 6.0424s
> 
> and there is initial duration from (moov.mvex.mehd duration 3622) /
> (moov.mvhd timescale 600) = 6,03666.
> 
> 
> * Mismatch buffered() part
> 
> It's the exact expected behavior after this patch, to use common
> intersection of ranges, which is 6.037, not 6.0424, so I believe that either
> this patch should should change `mediasource-util.js` "duration: 6.0424" to
> "duration: 6.037", or skip test and someone with Mac access would fix
> testcase later?
> I don't know how other tests will behave, my own mp4 parser doesn't handle
> trackId's yet.

I've had to modify those values (in mediasource-util.js) in the past, and it certainly seems reasonable to do so in this case.  Once you get another patch up, I can apply it and run them manually, and post the results.

> * Dom exception part
> 
> I'm not sure yet, but I suspect that this is because track2 extended
> duration to 6.042, and because of current "// FIXME: Re-number or update
> this section once" in streamEndedWithError() which reorders readystate
> change, setDurationInternal() uses buffered which was *not* extended to
> 'highest end time' (so 6.037, not 6.042) => calls setDurationinternal() with
> different old and new durations => sets updating to true =>
> "sourceBuffer.remove(1, 2);" has InvalidStateError, as it should wait for
> 'updateend'.

This is my understanding as well.

> This can be fixed by doing patch for that FIXME I mentioned (since that bug
> in spec already got resolved), but there could be perhaps check for is it
> safe to call remove?

I would opt for fixing the FIXME, rather than modifying the test in this case.

> Summing up, I'm not sure what should I do with this patch next. As for Jer
> Noble comment about nullifying buffered, instead of using additional flag -
> I'd like to change this in next patch with caching buffered for MediaSource. 
> 
> As I said I don't have Mac to properly change all those constants in test
> (like 1.029, 3.187, etc.) which probably will be off after this patch (so
> only changing segment duration to shorter version won't be enough).
> 
> 
> As a side note: I wonder shouldn't this be cleared in standard, because at
> the moment, when removal occurs, 'sourceended' being posted before
> 'updateend' seems wrong . I'd expect at 'sourceended' that if there was any
> removal it is done already, and updating is false, otherwise every user
> would have to add additional check before taking any action, for 'updating'
> which might or might not occur. I will probably ask this on W3 bugzila.