Bug 139813 - [MSE] Implement per TrackBuffer buffered.
Summary: [MSE] Implement per TrackBuffer buffered.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 140484 140622
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-19 01:59 PST by Bartlomiej Gajda
Modified: 2015-01-19 10:35 PST (History)
9 users (show)

See Also:


Attachments
patch_wip_per_trackbuffer_v1 (10.47 KB, patch)
2014-12-19 02:04 PST, Bartlomiej Gajda
no flags Details | Formatted Diff | Diff
add per trackBuffer buffered + layoutTests (19.54 KB, patch)
2015-01-14 05:02 PST, Bartlomiej Gajda
no flags Details | Formatted Diff | Diff
add per trackBuffer buffered + layoutTests v2 (25.01 KB, patch)
2015-01-14 10:55 PST, Bartlomiej Gajda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bartlomiej Gajda 2014-12-19 01:59:41 PST
Section 3.1 of MSE spec, regarding buffered() clearly indicates that it should be per trackBuffer, not per SourceBuffer.
Comment 1 Bartlomiej Gajda 2014-12-19 02:04:04 PST
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.
Comment 2 Bartlomiej Gajda 2014-12-19 02:07:26 PST
In the "reversed order" of course the first point (get intersection) would be cached value.
Comment 3 Jer Noble 2015-01-05 10:04:27 PST
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()?
Comment 4 Bartlomiej Gajda 2015-01-09 10:05:37 PST
> 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.
Comment 5 Bartlomiej Gajda 2015-01-14 05:02:44 PST
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.
Comment 6 Jer Noble 2015-01-14 08:35:16 PST
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().
Comment 7 Bartlomiej Gajda 2015-01-14 10:41:55 PST
> 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.
Comment 8 Bartlomiej Gajda 2015-01-14 10:55:02 PST
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.
Comment 9 Jer Noble 2015-01-14 11:54:03 PST
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.
Comment 10 WebKit Commit Bot 2015-01-14 12:36:25 PST
Comment on attachment 244617 [details]
add per trackBuffer buffered + layoutTests v2

Clearing flags on attachment: 244617

Committed r178438: <http://trac.webkit.org/changeset/178438>
Comment 11 WebKit Commit Bot 2015-01-14 12:36:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Joseph Pecoraro 2015-01-14 16:26:30 PST
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?
Comment 13 WebKit Commit Bot 2015-01-14 17:11:48 PST
Re-opened since this is blocked by bug 140484
Comment 14 Bartlomiej Gajda 2015-01-15 06:59:36 PST
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.
Comment 15 Jer Noble 2015-01-15 08:11:11 PST
(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.