1175768 - Run NotifyDataArrived on the reader task queue and mirror buffered ranges

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

This is the last big step towards eliminating the decoder monitor.

I have patches for the NotifyDataArrived stuff that are mostly green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adc9d9b13738

I have the .buffered stuff working for non-MSE, but I've run into some difficulties with MSE. Jean-Yves has been deep in that code for a while, so he's going to take a look.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make the logic in MDSM::NotifyDataArrived apply to mObservedDuration. v1

With this change, we don't need to condition this work on infinite streams.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Dispatch UpdateEstimatedMediaDuration. v1

NotifyDataArrived will soon run off-main-thread, so the assumptions here won't hold.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement SilentReadAt. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Dispatch NotifyDataArrived and remove the aBuffer argument. v1

It would be nice to remove the argument in a separate patch, but we can't
perform MediaResource reads on the main thread, so the SilentReadAt stuff
needs to happen at the same time as the off-main-thread stuff.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Misc changes to Intervals/TimeUnit. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Use mirroring for buffered ranges. WIP

This currently asserts for the MSE case.

Comment 7

[Bobby Holley (:bholley)]

NI jya to take a look at completing part 6.

Note that you'll need to make sure you have bug 1172264 applied (landed on inbound 8 hours ago, but hasn't merged to m-c yet).

Also note that the patches through Part 4 have a couple of failures on try [1], so don't worry if you run into those. Just get something more or less working if you can and I'll polish everything up tomorrow.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=adc9d9b13738

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623992 [details] [diff] [review]
Part 5 - Misc changes to Intervals/TimeUnit. v1

Review of attachment 8623992 [details] [diff] [review]:
-----------------------------------------------------------------

I wasn't asked for review, but somehow I feel I should have been :)

Comment 9

[Jean-Yves Avenard [:jya]]

Quick Note:
MediaDecoderReader::NotifyDataRemoved() must be handled the same way as NotifyDataArrived -> not running on the main thread.

We can't just remove the use of the main thread demuxer and stop notifying it that new data has been added or removed; as it's required for calculating the eviction offset which also is called on the main thread.

Only once those are also implemented in that fashion (maybe simply cache the eviction offset value when NotifyDataArrived / NotifyDataRemoved has been called) would we be able to remove the main thread demuxer.

Maybe time to remove MP4Reader while at it, because it's going to get in the way.

Comment 10

[Jean-Yves Avenard [:jya]]

I'm not sure where to put this or if this is related to WIP on this patch.

I've hit several crashes at various places.
One is MediaFormatReader::Shutdown() being called after MediaDecoderReader::mDecoder was set to null.
This causes a crash in MediaFormatReader::ReleaseMediaResource (but so would MP4Reader)
backtrace:
(lldb) print mDecoder
(mozilla::AbstractMediaDecoder *) $0 = 0x0000000000000000
(lldb) bt
* thread #100: tid = 0x72005c, 0x00000001065c3b67 XUL`mozilla::MediaFormatReader::ReleaseMediaResources(this=0x000000012d8a0000) + 23 at MediaFormatReader.cpp:1345, name = 'MediaPl~back #3', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001065c3b67 XUL`mozilla::MediaFormatReader::ReleaseMediaResources(this=0x000000012d8a0000) + 23 at MediaFormatReader.cpp:1345
    frame #1: 0x00000001065a6c3c XUL`mozilla::MediaDecoderReader::Shutdown(this=0x000000012d8a0000) + 188 at MediaDecoderReader.cpp:364
    frame #2: 0x00000001065bc542 XUL`mozilla::MediaFormatReader::Shutdown(this=0x000000012d8a0000) + 1250 at MediaFormatReader.cpp:146
    frame #3: 0x00000001067859fa XUL`mozilla::DelayedDispatchToMainThread::Run(this=0x00000001222070d0) + 58 at TrackBuffer.cpp:1150
    frame #4: 0x0000000106587b69 XUL`mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run(this=0x00000001355de120) + 281 at TaskDispatcher.h:181
    frame #5: 0x000000010669f62d XUL`mozilla::MediaTaskQueue::Runner::Run(this=0x000000012d25aa20) + 605 at MediaTaskQueue.cpp:256
    frame #6: 0x000000010371fa08 XUL`nsThreadPool::Run(this=0x00000001467448a0) + 984 at nsThreadPool.cpp:221
    frame #7: 0x000000010371fb0c XUL`non-virtual thunk to nsThreadPool::Run(this=0x00000001467448a8) + 28 at nsThreadPool.cpp:235
    frame #8: 0x000000010371c504 XUL`nsThread::ProcessNextEvent(this=0x0000000145ea13e0, aMayWait=false, aResult=0x000000012d501c5e) + 2068 at nsThread.cpp:848
    frame #9: 0x00000001037929f7 XUL`NS_ProcessNextEvent(aThread=0x0000000145ea13e0, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #10: 0x0000000103dc9e20 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x00000001465d9680, aDelegate=0x00000001460ef920) + 656 at MessagePump.cpp:326
    frame #11: 0x0000000103d372a5 XUL`MessageLoop::RunInternal(this=0x00000001460ef920) + 117 at message_loop.cc:234
    frame #12: 0x0000000103d371b5 XUL`MessageLoop::RunHandler(this=0x00000001460ef920) + 21 at message_loop.cc:227
    frame #13: 0x0000000103d3715d XUL`MessageLoop::Run(this=0x00000001460ef920) + 45 at message_loop.cc:201
    frame #14: 0x000000010371aa29 XUL`nsThread::ThreadFunc(aArg=0x0000000145ea13e0) + 329 at nsThread.cpp:360
    frame #15: 0x00000001033715d1 libnss3.dylib`_pt_root(arg=0x0000000146020410) + 449 at ptthread.c:212
    frame #16: 0x00007fff9384a268 libsystem_pthread.dylib`_pthread_body + 131
    frame #17: 0x00007fff9384a1e5 libsystem_pthread.dylib`_pthread_start + 176
    frame #18: 0x00007fff9384841d libsystem_pthread.dylib`thread_start + 13
(lldb) 

One encountered a lot is:
the assertion is: 
    MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));


* thread #1: tid = 0x641d60, 0x0000000106681dcd XUL`mozilla::MediaTaskQueue::Dispatch(this=0x000000012ab09010, aRunnable=(mRawPtr = 0x0000000000000000), aFailureHandling=AssertDispatchSuccess, aReason=TailDispatch) + 237 at MediaTaskQueue.h:53, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000106681dcd XUL`mozilla::MediaTaskQueue::Dispatch(this=0x000000012ab09010, aRunnable=(mRawPtr = 0x0000000000000000), aFailureHandling=AssertDispatchSuccess, aReason=TailDispatch) + 237 at MediaTaskQueue.h:53
    frame #1: 0x0000000106586a18 XUL`mozilla::AutoTaskDispatcher::DispatchTaskGroup(this=0x0000000117e4d188, aGroup=UniquePtr<mozilla::AutoTaskDispatcher::PerThreadTaskGroup, mozilla::DefaultDelete<mozilla::AutoTaskDispatcher::PerThreadTaskGroup> > at 0x00007fff5fbfcbb8) + 200 at TaskDispatcher.h:233
    frame #2: 0x0000000106588098 XUL`mozilla::AutoTaskDispatcher::~AutoTaskDispatcher(this=0x0000000117e4d188) + 232 at TaskDispatcher.h:88
    frame #3: 0x0000000106586115 XUL`mozilla::AutoTaskDispatcher::~AutoTaskDispatcher(this=0x0000000117e4d188) + 21 at TaskDispatcher.h:75
    frame #4: 0x000000010658568f XUL`mozilla::Maybe<mozilla::AutoTaskDispatcher>::reset(this=0x0000000117e4d180) + 63 at Maybe.h:373
    frame #5: 0x0000000106585981 XUL`mozilla::XPCOMThreadWrapper::FireTailDispatcher(this=0x0000000117e4d160) + 161 at AbstractThread.cpp:76
    frame #6: 0x0000000106585eb3 XUL`void nsRunnableMethodArguments<>::apply<mozilla::XPCOMThreadWrapper, void (this=0x000000012b77cff0, o=0x0000000117e4d160, m=0x00000001065858e0)()>(mozilla::XPCOMThreadWrapper*, void (mozilla::XPCOMThreadWrapper::*)()) + 131 at nsThreadUtils.h:618
    frame #7: 0x0000000106585b4c XUL`nsRunnableMethodImpl<void (mozilla::XPCOMThreadWrapper::*)(), true>::Run(this=0x000000012b77cfc0) + 140 at nsThreadUtils.h:825
    frame #8: 0x00000001070670fe XUL`nsBaseAppShell::RunSyncSectionsInternal(this=0x000000011f277660, aStable=true, aThreadRecursionLevel=0) + 462 at nsBaseAppShell.cpp:376
    frame #9: 0x000000010707a31d XUL`nsBaseAppShell::RunSyncSections(this=0x000000011f277660, stable=true, threadRecursionLevel=0) + 77 at nsBaseAppShell.h:95
    frame #10: 0x00000001070673bd XUL`nsBaseAppShell::AfterProcessNextEvent(this=0x000000011f277660, thr=0x0000000100431310, recursionDepth=0, eventWasProcessed=true) + 45 at nsBaseAppShell.cpp:435
    frame #11: 0x00000001070dc893 XUL`nsAppShell::AfterProcessNextEvent(this=0x000000011f277660, aThread=0x0000000100431310, aRecursionDepth=0, aEventWasProcessed=true) + 307 at nsAppShell.mm:762
    frame #12: 0x00000001070dc944 XUL`non-virtual thunk to nsAppShell::AfterProcessNextEvent(this=0x000000011f277668, aThread=0x0000000100431310, aRecursionDepth=0, aEventWasProcessed=true) + 68 at nsAppShell.mm:766
    frame #13: 0x000000010371be24 XUL`nsThread::ProcessNextEvent(this=0x0000000100431310, aMayWait=false, aResult=0x00007fff5fbfd063) + 2596 at nsThread.cpp:862
    frame #14: 0x0000000103791f67 XUL`NS_ProcessPendingEvents(aThread=0x0000000100431310, aTimeout=20) + 151 at nsThreadUtils.cpp:207
    frame #15: 0x0000000107066656 XUL`nsBaseAppShell::NativeEventCallback(this=0x000000011f277660) + 198 at nsBaseAppShell.cpp:99
    frame #16: 0x00000001070db68d XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x000000011f277660) + 445 at nsAppShell.mm:378
    frame #17: 0x00007fff8f425a01 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #18: 0x00007fff8f417b8d CoreFoundation`__CFRunLoopDoSources0 + 269
    frame #19: 0x00007fff8f4171bf CoreFoundation`__CFRunLoopRun + 927
    frame #20: 0x00007fff8f416bd8 CoreFoundation`CFRunLoopRunSpecific + 296
    frame #21: 0x00007fff8f8fb56f HIToolbox`RunCurrentEventLoopInMode + 235
    frame #22: 0x00007fff8f8fb2ea HIToolbox`ReceiveNextEventCommon + 431
    frame #23: 0x00007fff8f8fb12b HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #24: 0x00007fff8e5d59bb AppKit`_DPSNextEvent + 978
    frame #25: 0x00007fff8e5d4f68 AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
    frame #26: 0x00000001070da1b7 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x0000000117e4d4c0, _cmd=0x00007fff8eef1808, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x00007fff7d444040, flag='\x01') + 119 at nsAppShell.mm:119
    frame #27: 0x00007fff8e5cabf3 AppKit`-[NSApplication run] + 594
    frame #28: 0x00000001070dc061 XUL`nsAppShell::Run(this=0x000000011f277660) + 177 at nsAppShell.mm:653
    frame #29: 0x00000001080fb831 XUL`nsAppStartup::Run(this=0x000000011f2790b0) + 161 at nsAppStartup.cpp:280
    frame #30: 0x00000001081c54cb XUL`XREMain::XRE_mainRun(this=0x00007fff5fbfefe0) + 5963 at nsAppRunner.cpp:4263
    frame #31: 0x00000001081c5ded XUL`XREMain::XRE_main(this=0x00007fff5fbfefe0, argc=5, argv=0x00007fff5fbff8e0, aAppData=0x00007fff5fbff268) + 989 at nsAppRunner.cpp:4347
    frame #32: 0x00000001081c6292 XUL`XRE_main(argc=5, argv=0x00007fff5fbff8e0, aAppData=0x00007fff5fbff268, aFlags=0) + 98 at nsAppRunner.cpp:4436
    frame #33: 0x00000001000029fb firefox`do_main(argc=5, argv=0x00007fff5fbff8e0, xreDirectory=0x000000010040df80) + 1819 at nsBrowserApp.cpp:214
    frame #34: 0x0000000100001dcf firefox`main(argc=5, argv=0x00007fff5fbff8e0) + 287 at nsBrowserApp.cpp:478
    frame #35: 0x0000000100001854 firefox`start + 52

this is mostly seen on the youtube MSE conformance test. Dispatch failure, would that mean that the target task queue has been shutdown while there are still tail dispatch task pending ???

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: . P6-2 Remove now unused members.

The main thread demuxer still needs to be retained at this stage, in order to properly calculate GetEvictionOffset() this could also be reworked to work on the reader's thread.

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: P6-3. Cache SourceBuffer buffered ranges.

Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: P6-4. Cache SourceBuffer buffered ranges.

Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.

Comment 14

[Jean-Yves Avenard [:jya]]

All of the old MSE is handled I believe.

about:media plugin is broken with this as it calls GetBuffered() on the main thread to build its string.

If the buffered range of all sub-readers were mirrored, we could read it that way ; or do the same dirty trick as I did in patch P6-4 and query all trackbuffers until we found the buffered ranges for the required decoder...

Or better, get rid of this MSE/MediaSourceReader and use the new one :)

Comment 15

[Bobby Holley (:bholley)]

Thanks for helping move this forward!

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> I'm not sure where to put this or if this is related to WIP on this patch.

Yeah, let's keep them separate for now, and then roll all of the part 6 pieces together before landing.

> I've hit several crashes at various places.
> One is MediaFormatReader::Shutdown() being called after
> MediaDecoderReader::mDecoder was set to null.

This is puzzling to me (and I can't reproduce it). In your stack, The call to ReleaseMediaResources is coming from MediaDecoderReader::Shutdown, and occurs _before_ the part where we set mDecoder = nullptr. Does this still reproduce?

> One encountered a lot is:
> the assertion is: 
>     MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess ||
> NS_SUCCEEDED(rv));

Fixed this issue in the updated patches I'm about to upload - DispatchNotifyDataArrived shouldn't assert dispatch success.

Comment 16

[Bobby Holley (:bholley)]

I now get the following crash on the YouTube suite:

Assertion failure: !Exists(), at /files/mozilla/repos/dd/dom/media/MediaPromise.h:818
#01: mozilla::MediaPromiseRequestHolder<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true> >::Begin(mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true>::Request*)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3062fc5]
#02: mozilla::TrackBuffer::InitializeDecoder(mozilla::SourceBufferDecoder*)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x31cf50f]
#03: void nsRunnableMethodArguments<mozilla::SourceBufferDecoder*>::apply<mozilla::TrackBuffer, void (mozilla::TrackBuffer::*)(mozilla::SourceBufferDecoder*)>(mozilla::TrackBuffer*, void (mozilla::TrackBuffer::*)(mozilla::SourceBufferDecoder*))[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x31f17cf]
#04: nsRunnableMethodImpl<void (mozilla::TrackBuffer::*)(mozilla::SourceBufferDecoder*), true, mozilla::SourceBufferDecoder*>::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x31f14fc]
#05: mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3002ca9]
#06: mozilla::MediaTaskQueue::Runner::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x311a8fd]
#07: nsThreadPool::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x19a998]
#08: non-virtual thunk to nsThreadPool::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x19aa9c]
#09: nsThread::ProcessNextEvent(bool, bool*)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x197494]
#10: NS_ProcessNextEvent(nsIThread*, bool)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x20d9d7]
#11: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x84510e]
#12: MessageLoop::RunInternal()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7b2485]
#13: MessageLoop::RunHandler()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7b2395]
#14: MessageLoop::Run()[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7b233d]
#15: nsThread::ThreadFunc(void*)[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1959b9]
#16: _pt_root[/files/mozilla/repos/dd/obj-x86_64-apple-darwin14.3.0/dist/NightlyDebug.app/Contents/MacOS/libnss3.dylib +0x3718d1]
#17: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x3268]
#18: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x31e5]
Process 29802 stopped
* thread #136: tid = 0x951993, 0x00000001045e7fc9 XUL`mozilla::MediaPromiseRequestHolder<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true> >::Begin(this=0x000000011e227b10, aRequest=0x0000000127fa2160) + 89 at MediaPromise.h:818, name = 'MediaPl~back #1', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001045e7fc9 XUL`mozilla::MediaPromiseRequestHolder<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true> >::Begin(this=0x000000011e227b10, aRequest=0x0000000127fa2160) + 89 at MediaPromise.h:818
   815 	
   816 	  void Begin(typename PromiseType::Request* aRequest)
   817 	  {
-> 818 	    MOZ_DIAGNOSTIC_ASSERT(!Exists());
   819 	    mRequest = aRequest;
   820 	  }
   821 	

This is deep in TrackBuffer, so I'll leave investigation to you.

I'm also getting one failure in our MSE test suite:

dom/media/mediasource/test/test_TruncatedDuration.html | sourcebuffer truncated - got 4.001, expected 1.334

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 4 - Dispatch NotifyDataArrived and remove the aBuffer argument. v2

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 6-1 - Dispatch NotifyDataArrived and remove the aBuffer argument. v2

Comment 19

[Bobby Holley (:bholley)]

Attachment: Misc Fix - Null-check mInputBuffer. v1

This probably isn't the right thing, but I needed this to avoid null derefs.

Comment 20

[Bobby Holley (:bholley)]

Let's see what else try says:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b2302c2480

Comment 21

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fd2a0cf3165

Comment 22

[Bobby Holley (:bholley)]

Attachment: Misc Fixes. v2

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624494 [details] [diff] [review]
Misc Fixes. v2

Review of attachment 8624494 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/TrackBuffer.cpp
@@ +545,5 @@
>    }
>  
>    if (evicted) {
>      ProxyMediaCall(mParentDecoder->GetReader()->TaskQueue(), this, __func__,
> +                   &TrackBuffer::UpdateBufferedRanges, uint32_t(0), int64_t(0));

don't you like the original K&R C ??

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 6-1 - Use mirroring for buffered ranges. P6-1 v2

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement SilentReadAt. v2

This fixes the test_playback_rate failure.

Comment 26

[Bobby Holley (:bholley)]

Attachment: Misc Fixes. v3

This includes an 'explicit' to make the static analysis happy.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove bogus check. r=jya

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: P6-5. Misc Fix #2

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 4 - Dispatch NotifyDataArrived and remove the aBuffer argument. v3

This should hopefully fix the b2g failures.

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: P6-3. Cache SourceBuffer buffered ranges.

Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: P6-4. Cache SourceBuffer buffered ranges.

Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624569 [details] [diff] [review]
P6-5. Misc Fix #2

folded across P6-3 and P6-4

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623988 [details] [diff] [review]
Part 1 - Make the logic in MDSM::NotifyDataArrived apply to mObservedDuration. v1

Review of attachment 8623988 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ -1646,5 @@
> -  if (!mDecoder->IsInfinite() || !HaveStartTime())
> -  {
> -    return;
> -  }
> -

Removing this will cause bug 1166836 to come back.
I too thought it could be removed and it bit me big time.

NotifyDataArrived() is called every 32kB (or maybe 64 can't remember) the MediaResource download data.
With a long MP4 this causes the process to recalculate the getbuffered range.

I just tried the video in that bug and indeed, seeing 180%+ CPU usage, and NotifyDemuxer being called thousand of times in a row.
Now the situation is better because it's not done on the main thread anymore, but it's still unnecessary.

This particular code is just a dirty hack IMHO, and only there to ensure that we have at least once the buffered range calculated. So we should only run this code once and if it has run before, skip it.

Comment 34

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623989 [details] [diff] [review]
Part 2 - Dispatch UpdateEstimatedMediaDuration. v1

Review of attachment 8623989 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623989 [details] [diff] [review]
Part 2 - Dispatch UpdateEstimatedMediaDuration. v1

Review of attachment 8623989 [details] [diff] [review]:
-----------------------------------------------------------------

Of course \o/ meant r+

Comment 36

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624560 [details] [diff] [review]
Part 6-1 - Use mirroring for buffered ranges. P6-1 v2

Review of attachment 8624560 [details] [diff] [review]:
-----------------------------------------------------------------

I'll wait for all 6 to be reviewed and at least P6-2 other you have a regression in how GetEvictionOffset will return invalid data. how it is currently done: it can't work

Comment 37

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624565 [details] [diff] [review]
Part 3 - Implement SilentReadAt. v2

Review of attachment 8624565 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaResource.h
@@ +292,5 @@
> +
> +  // ReadAt without side-effects. Given that our MediaResource infrastructure
> +  // is very side-effecty, this accomplishes its job by checking the initial
> +  // position and seeking back to it. If the seek were to fail, a side-effect
> +  // might be observable.

it seems rather pointless to seek back; especially if you have two thread attempt to read the mediaresource at the same time; there's no guarantee anyway that Tell() will return anything meaningfull.

AFAIK, only the WebM reader does that, and this is why we can't use it with MSE as it's not thread-safe.

@@ +298,5 @@
> +  // This method returns null if anything fails, including the failure to read
> +  // aCount bytes. Otherwise, it returns an owned buffer.
> +  virtual mozilla::UniquePtr<char[]> SilentReadAt(int64_t aOffset, uint32_t aCount)
> +  {
> +    mozilla::UniquePtr<char[]> bytes(new (fallible) char[aCount]);

why not return an already_addRefed<MediaByteBuffer> (and if you want to ensure there's ownership transfer return a nsRefPtr<MediaByteBuffer>
call SetCapacity with fallible as 2nd argument

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: . P6-2 Remove now unused members.

The main thread demuxer still needs to be retained at this stage, in order to properly calculate GetEvictionOffset() this could also be reworked to work on the reader's thread.

Comment 39

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624583 [details] [diff] [review]
. P6-2 Remove now unused members.

actually no, it won't help...

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 8623988 [details] [diff] [review]
Part 1 - Make the logic in MDSM::NotifyDataArrived apply to mObservedDuration. v1

Per IRC discussion, this method actually goes away entirely by the end of the patch stack. What we need is potentially a way to throttle NotifyDataArrived notifications for the reader, which I'll look into tomorrow.

Comment 41

[Jean-Yves Avenard [:jya]]

Comment on attachment 8623988 [details] [diff] [review]
Part 1 - Make the logic in MDSM::NotifyDataArrived apply to mObservedDuration. v1

Review of attachment 8623988 [details] [diff] [review]:
-----------------------------------------------------------------

several possibilities to reduce CPU usage.

In MediaFormatReader, only schedule an update is mEOS or mWaitingForData is true.

Buffer the NotifyDataReceived in the same fashion I did (Code that was removed in part6). That is keep a TimeInterval that is updated with the span of each call to NotifyDataArrived and schedule an update if EOS or Waiting for data or if last NotifyDataArrived was more than say 1s.

Have the MDSM perform a similar buffering.. We don't need to know that there's new data that is coming that often...

Comment 42

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #37)
> it seems rather pointless to seek back; especially if you have two thread
> attempt to read the mediaresource at the same time; there's no guarantee
> anyway that Tell() will return anything meaningfull.

In principal I agree with you - the API should only provide Read, not ReadAt. But I want to minimize the risk of weird regressions, so I'm just doing it for now. We can fix it when we remove Read() and Tell() from MediaResource.

> why not return an already_addRefed<MediaByteBuffer> (and if you want to
> ensure there's ownership transfer return a nsRefPtr<MediaByteBuffer>
> call SetCapacity with fallible as 2nd argument

Done - turned out to be a lot of work though, especially because of the type mismatch between char* and unsigned char*.

Comment 43

[Bobby Holley (:bholley)]

Attachment: patches.zip

So, this seems to be more orange than before on the mediasource mochitests and webref:

Last night's push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5135e30a068a

Today's push, with a fix for the webm crash:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cadc97732eec

Attaching my complete current patch stack, rebased against trunk. Jean-Yves, can you take a look at the MSE failures?

Comment 44

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #41)
> Comment on attachment 8623988 [details] [diff] [review]
> Part 1 - Make the logic in MDSM::NotifyDataArrived apply to
> mObservedDuration. v1
> 
> Review of attachment 8623988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> several possibilities to reduce CPU usage.
> 
> In MediaFormatReader, only schedule an update is mEOS or mWaitingForData is
> true.
> 
> Buffer the NotifyDataReceived in the same fashion I did (Code that was
> removed in part6). That is keep a TimeInterval that is updated with the span
> of each call to NotifyDataArrived and schedule an update if EOS or Waiting
> for data or if last NotifyDataArrived was more than say 1s.

I'm confused - you're suggesting that the primary overhead is in NotifyDemuxer and the associated Update? That overhead exists before the patch as well. It seems to me that the problem is the UpdateBuffered call in MediaDecoderReader::NotifyDataArrived. I'll write up a patch to throttle that.

Comment 45

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #44)
> I'm confused - you're suggesting that the primary overhead is in
> NotifyDemuxer and the associated Update? That overhead exists before the
> patch as well. It seems to me that the problem is the UpdateBuffered call in
> MediaDecoderReader::NotifyDataArrived. I'll write up a patch to throttle
> that.

Oh, it looks like the NotifyDataArrived implementation in MediaFormatReader also invokes GetBuffered. So I'll throttle both in my patch.

Comment 46

[Bobby Holley (:bholley)]

Attachment: Throttle NotifyDataArrived. v1

This reduces the time spent in GetBuffered on the channel9 video from ~40% to
~1.5% on my machine.

Comment 47

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #43)
> Today's push, with a fix for the webm crash:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cadc97732eec

Looks like the remaining failures are MSE-related, plus some omx stuff. I've tweaked the OMX stuff, and added the throttling. Hopefully with this push we'll be down to just the MSE failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd05cbf006c

Comment 48

[Jean-Yves Avenard [:jya]]

Comment on attachment 8625008 [details] [diff] [review]
Throttle NotifyDataArrived. v1

Review of attachment 8625008 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.h
@@ +258,5 @@
> +      return *this;
> +    }
> +
> +    uint32_t mLength;
> +    int64_t mOffset;

all of this could be more elegantly done with an Interval<int64_t> (or MediaByteRange)

Check how I did it in MediaFormatReader before patch 6.

All ranges provided in NotifyDataArrived are incrementals so we can simply span one interval after another.

::: dom/media/mediasource/TrackBuffer.cpp
@@ +318,5 @@
>          // XXX: Params make no sense to parent decoder as it relates to a
>          // specific SourceBufferDecoder's data stream.  Pass bogus values here to
>          // force parent decoder's state machine to recompute end time for
>          // infinite length media.
> +        parent->NotifyDataArrived(0, 0, /* aThrottleUpdates = */ false);

we actually can get rid of that one as it's done in SourceBuffer now

Comment 49

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (:bholley) from comment #45)
> (In reply to Bobby Holley (:bholley) from comment #44)
> > I'm confused - you're suggesting that the primary overhead is in
> > NotifyDemuxer and the associated Update? That overhead exists before the
> > patch as well. It seems to me that the problem is the UpdateBuffered call in
> > MediaDecoderReader::NotifyDataArrived. I'll write up a patch to throttle
> > that.
> 
> Oh, it looks like the NotifyDataArrived implementation in MediaFormatReader
> also invokes GetBuffered. So I'll throttle both in my patch.

This used to be implicitly throttled by ScheduleUpdate as it would only schedule an update if none were pending. This significantly reduced the load compare to how the MP4Reader was doing it.

We only need to perform the GetBuffered if we are in EOS or WAITING_FOR_DATA, can look into implementing this in another patch.

But I think the throttling based on the last time is sufficient enough.

Comment 50

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (:bholley) from comment #47)
> (In reply to Bobby Holley (:bholley) from comment #43)
> > Today's push, with a fix for the webm crash:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cadc97732eec
> 
> Looks like the remaining failures are MSE-related, plus some omx stuff. I've
> tweaked the OMX stuff, and added the throttling. Hopefully with this push
> we'll be down to just the MSE failures.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd05cbf006c

We have the assertions about the mOffset != aOffset

This could be removed by using Interval<int64_t>and use Interval.Span()

Comment 51

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #50)
> (In reply to Bobby Holley (:bholley) from comment #47)
> > (In reply to Bobby Holley (:bholley) from comment #43)
> > > Today's push, with a fix for the webm crash:
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cadc97732eec
> > 
> > Looks like the remaining failures are MSE-related, plus some omx stuff. I've
> > tweaked the OMX stuff, and added the throttling. Hopefully with this push
> > we'll be down to just the MSE failures.
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd05cbf006c
> 
> We have the assertions about the mOffset != aOffset
> 
> This could be removed by using Interval<int64_t>and use Interval.Span()

Sure, I'll fix that. Still need to fix the MSE issues though - easy to reproduce by just not applying the throttling patch.

Comment 52

[Bobby Holley (:bholley)]

Attachment: patches.zip

Comment 53

[Bobby Holley (:bholley)]

Attachment: patches.zip (git)

Comment 54

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #48)
> all of this could be more elegantly done with an Interval<int64_t> (or
> MediaByteRange)

Fixed to use Interval<int64_t>.

> we actually can get rid of that one as it's done in SourceBuffer now

Let's save that for a followup.

Comment 55

[Bobby Holley (:bholley)]

Attachment: Throttle NotifyDataArrived. r=jya

Comment 56

[Bobby Holley (:bholley)]

With a fixed up throttling patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec6018b5bc7c

Comment 57

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (:bholley) from comment #54)
> (In reply to Jean-Yves Avenard [:jya] from comment #48)
> > all of this could be more elegantly done with an Interval<int64_t> (or
> > MediaByteRange)
> 
> Fixed to use Interval<int64_t>.
> 
> > we actually can get rid of that one as it's done in SourceBuffer now
> 
> Let's save that for a followup.
 But then you don't get contiguous intervals passed to NotifyDataArrived (always 0,0)

Comment 58

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #57)
>  But then you don't get contiguous intervals passed to NotifyDataArrived
> (always 0,0)

That line passes aThrottle = false, so it's unaffected by that codepath.

(In reply to Bobby Holley (:bholley) from comment #56)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec6018b5bc7c

This is now green everywhere modulo the MSE failures. Over to jya for the last piece.

Comment 59

[Bobby Holley (:bholley)]

Attachment: patches.zip (git)

Fully updated patches, including the throttling patch.

Comment 60

[Jean-Yves Avenard [:jya]]

Comment on attachment 8625020 [details] [diff] [review]
Throttle NotifyDataArrived. r=jya

Review of attachment 8625020 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.cpp
@@ +185,5 @@
> +  if (mThrottledInterval.isNothing()) {
> +    mThrottledInterval.emplace(aInterval);
> +  } else if (!mThrottledInterval.ref().Contiguous(aInterval)) {
> +    DoThrottledNotify();
> +    mThrottledInterval.emplace(aInterval);

if you have any pending mThrottledInterface you could lose it no? shouldn't you dispatch the earlier one (if any) and then schedule this one?

Comment 61

[Jean-Yves Avenard [:jya]]

Attachment: P6-3. Cache SourceBuffer buffered ranges.

Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.

Comment 62

[Jean-Yves Avenard [:jya]]

Attachment: P6-4. Cache SourceBuffer buffered ranges.

Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.

Comment 63

[Jean-Yves Avenard [:jya]]

Attachment: Throttle NotifyDataArrived.

Comment 64

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e643400ae8b

Comment 65

[Jean-Yves Avenard [:jya]]

Missed one:
(lldb) bt
* thread #1: tid = 0x13e697, 0x000000010675843c XUL`mozilla::MediaSourceReader::GetBuffered(this=0x0000000130311800) + 140 at MediaSourceReader.cpp:1013, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x000000010675843c XUL`mozilla::MediaSourceReader::GetBuffered(this=0x0000000130311800) + 140 at MediaSourceReader.cpp:1013
    frame #1: 0x000000010674ce6f XUL`mozilla::MediaSourceDecoder::GetSeekable(this=0x0000000125382640) + 367 at MediaSourceDecoder.cpp:90
  * frame #2: 0x0000000106448a5e XUL`mozilla::dom::HTMLMediaElement::Seek(this=0x0000000135e43800, aTime=4884.0720000000001, aSeekType=Accurate, aRv=0x00007fff5fbf49e8) + 942 at HTMLMediaElement.cpp:1491
    frame #3: 0x0000000106448fbe XUL`mozilla::dom::HTMLMediaElement::SetCurrentTime(this=0x0000000135e43800, aCurrentTime=4884.0720000000001, aRv=0x00007fff5fbf49e8) + 46 at HTMLMediaElement.cpp:1401
    frame #4: 0x0000000105f9259a XUL`mozilla::dom::HTMLMediaElementBinding::set_currentTime(cx=0x00000001303de850, obj=Handle<JSObject *> at 0x00007fff5fbf4a30, self=0x0000000135e43800, args=JSJitSetterCallArgs at 0x00007fff5fbf4a28) + 186 at HTMLMediaElementBinding.cpp:480
    frame #5: 0x00000001060d4a82 XUL`mozilla::dom::GenericBindingSetter(cx=0x00000001303de850, argc=1, vp=0x00007fff5fbf5598) + 706 at BindingUtils.cpp:2526
    frame #6: 0x00000001094e75c8 XUL`js::CallJSNative(cx=0x00000001303de850, native=0x00000001060d47c0, args=0x00007fff5fbf5430)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 184 at jscntxtinlines.h:235
    frame #7: 0x0000000109475c6e XUL`js::Invoke(cx=0x00000001303de850, args=CallArgs at 0x00007fff5fbf5430, construct=NO_CONSTRUCT) + 1358 at Interpreter.cpp:709
    frame #8: 0x000000010945b9e6 XUL`js::Invoke(cx=0x00000001303de850, thisv=0x00007fff5fbf5c78, fval=0x00007fff5fbf5680, argc=1, argv=0x00007fff5fbf5ae0, rval=JS::MutableHandleValue at 0x00007fff5fbf5530) + 902 at Interpreter.cpp:766
    frame #9: 0x000000010949da58 XUL`js::InvokeSetter(cx=0x00000001303de850, thisv=0x00007fff5fbf5c78, fval=Value at 0x00007fff5fbf5680, v=JS::HandleValue at 0x00007fff5fbf5678) + 232 at Interpreter.cpp:851
    frame #10: 0x000000010958e36a XUL`SetExistingProperty(cx=0x00000001303de850, obj=js::HandleNativeObject at 0x00007fff5fbf5890, id=JS::HandleId at 0x00007fff5fbf5888, v=JS::HandleValue at 0x00007fff5fbf5880, receiver=JS::HandleValue at 0x00007fff5fbf5878, pobj=js::HandleNativeObject at 0x00007fff5fbf5870, shape=js::HandleShape at 0x00007fff5fbf5868, result=0x00007fff5fbf5c50) + 1914 at NativeObject.cpp:2288
    frame #11: 0x000000010958d866 XUL`js::NativeSetProperty(cx=0x00000001303de850, obj=js::HandleNativeObject at 0x00007fff5fbf5b20, id=JS::HandleId at 0x00007fff5fbf5b18, value=JS::HandleValue at 0x00007fff5fbf5b10, receiver=JS::HandleValue at 0x00007fff5fbf5b08, qualified=Qualified, result=0x00007fff5fbf5c50) + 838 at NativeObject.cpp:2322
    frame #12: 0x000000010954ff73 XUL`js::SetProperty(cx=0x00000001303de850, obj=JS::HandleObject at 0x00007fff5fbf5bc0, id=JS::HandleId at 0x00007fff5fbf5bb8, v=JS::HandleValue at 0x00007fff5fbf5bb0, receiver=JS::HandleValue at 0x00007fff5fbf5ba8, result=0x00007fff5fbf5c50) + 227 at NativeObject.h:1434
    frame #13: 0x00000001094b192b XUL`SetPropertyOperation(cx=0x00000001303de850, op=JSOP_SETPROP, lval=JS::HandleValue at 0x00007fff5fbf5ce0, id=JS::HandleId at 0x00007fff5fbf5cd8, rval=JS::HandleValue at 0x00007fff5fbf5cd0) + 491 at Interpreter.cpp:315
    frame #14: 0x000000010948e6eb XUL`Interpret(cx=0x00000001303de850, state=0x00007fff5fbf8d20) + 47563 at Interpreter.cpp:2767
    frame #15: 0x0000000109482c22 XUL`js::RunScript(cx=0x00000001303de850, state=0x00007fff5fbf8d20) + 706 at Interpreter.cpp:653
    frame #16: 0x0000000109475dac XUL`js::Invoke(cx=0x00000001303de850, args=CallArgs at 0x00007fff5fbf9520, construct=NO_CONSTRUCT) + 1676 at Interpreter.cpp:729
    frame #17: 0x0000000109c0b29a XUL`js::fun_apply(cx=0x00000001303de850, argc=2, vp=0x00007fff5fbfa8f8) + 1706 at jsfun.cpp:1289
    frame #18: 0x00000001094e75c8 XUL`js::CallJSNative(cx=0x00000001303de850, native=0x0000000109c0abf0, args=0x00007fff5fbfa790)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 184 at jscntxtinlines.h:235
    frame #19: 0x0000000109475c6e XUL`js::Invoke(cx=0x00000001303de850, args=CallArgs at 0x00007fff5fbfa790, construct=NO_CONSTRUCT) + 1358 at Interpreter.cpp:709
    frame #20: 0x000000010945b9e6 XUL`js::Invoke(cx=0x00000001303de850, thisv=0x00007fff5fbfab90, fval=0x00007fff5fbfabc0, argc=2, argv=0x00007fff5fbfad28, rval=JS::MutableHandleValue at 0x00007fff5fbfa890) + 902 at Interpreter.cpp:766
    frame #21: 0x00000001097f41a9 XUL`js::jit::DoCallFallback(cx=0x00000001303de850, frame=0x00007fff5fbfadc8, stub_=0x000000012742f890, argc=2, vp=0x00007fff5fbfad18, res=JS::MutableHandleValue at 0x00007fff5fbfac78) + 1849 at BaselineIC.cpp:9855
    frame #22: 0x000000011769320b

When seeking, HTMLMediaElement does media::TimeIntervals seekableIntervals = mDecoder->GetSeekable(); on the main thread, which calls MediaSourceDecoder::GetBuffered().

Everything in there is thread safe provided you hold the decoder's monitor.

GetSeekable should use the buffered mirror no?

surprising that none of the mochitest/webref triggers it. i got it trying this page: http://steamcommunity.com/broadcast/watch/76561198047709840

Comment 66

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624560 [details] [diff] [review]
Part 6-1 - Use mirroring for buffered ranges. P6-1 v2

Review of attachment 8624560 [details] [diff] [review]:
-----------------------------------------------------------------

In MediaSourceDecoder, changing GetSeekable so it calls GetBuffered rather than mReader->GetBuffered() is required (to prevent assert).

i have the feeling that the MSE is still wrong in how it's going to report the mediasource buffered range. if mediasource go into ended mode, it must update the buffered ranges as there are rules on what the buffered attribute is supposed to return when the mediasource is ended (all trackbuffers' end are aligned with the longest)

::: dom/media/MediaFormatReader.cpp
@@ +1311,5 @@
> +  if (HasAudio()) {
> +    UpdateReceivedNewData(TrackType::kAudioTrack);
> +  }
> +  if (HasVideo()) {
> +    MonitorAutoLock lock(mVideo.mMonitor);

the lock isn't required anymore

@@ +1315,5 @@
> +    MonitorAutoLock lock(mVideo.mMonitor);
> +    videoti = mVideo.mTimeRanges;
> +  }
> +  if (HasAudio()) {
> +    MonitorAutoLock lock(mAudio.mMonitor);

same with this one.
doc in MediaFormatReader.h should be amended, and definition of mTimeRanges moved up.

Comment 67

[Jean-Yves Avenard [:jya]]

ah! indeed:
 TEST-UNEXPECTED-FAIL | /media-source/mediasource-buffered.html | Demuxed content with different lengths - assert_equals: mediaElement.buffered expected "{ [0.000, 2.043) }" but got "{ [0.000, 2.000) }"

that's the case I described above...

when going into ended mode, we could dispatch a task that update the buffered ranges. however, i believe it's supposed to be synchronous. not sure on how that can be done now.

looking at the MSE spec, we are to run the range removal algorithm (which is async), but here we would have nothing to remove ; it's a bit blurry as such if the buffered attributes is to be updated immediately or not.

Comment 68

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #60)
> ::: dom/media/MediaDecoderReader.cpp
> @@ +185,5 @@
> > +  if (mThrottledInterval.isNothing()) {
> > +    mThrottledInterval.emplace(aInterval);
> > +  } else if (!mThrottledInterval.ref().Contiguous(aInterval)) {
> > +    DoThrottledNotify();
> > +    mThrottledInterval.emplace(aInterval);
> 
> if you have any pending mThrottledInterface you could lose it no? shouldn't
> you dispatch the earlier one (if any) and then schedule this one?

That's exactly what this code does, unless I'm misunderstanding something.

(In reply to Jean-Yves Avenard [:jya] from comment #66)
> doc in MediaFormatReader.h should be amended, and definition of mTimeRanges
> moved up.

By this I assume you mean removing mMonitor entirely, and the one other place where we acquire it (in MediaFormatReader::UpdateReceivedNewData), right? I'll do that, but please let me know if that's wrong for some reason.

Comment 69

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #67)
> looking at the MSE spec, we are to run the range removal algorithm (which is
> async), but here we would have nothing to remove ; it's a bit blurry as such
> if the buffered attributes is to be updated immediately or not.

Yeah - the spec indicates that we should update the duration, but I don't see anything in the spec that indicates that .buffered should be updated synchronously. Do you?

Comment 70

[Bobby Holley (:bholley)]

Attachment: Use mirroring for buffered ranges. r=jya

Comment 71

[Bobby Holley (:bholley)]

Attachment: Mark probably-invalid webref test as failing. v1

Comment 72

[Bobby Holley (:bholley)]

Comment on attachment 8625161 [details] [diff] [review]
Use mirroring for buffered ranges. r=jya

Just double-checking that I did the right thing wrt the monitor.

Comment 73

[Bobby Holley (:bholley)]

Final all-platform try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a60e71513dc

Comment 74

[Bobby Holley (:bholley)]

Attachment: patches.zip (git)

Rebased patches.

Comment 75

[Jean-Yves Avenard [:jya]]

Comment on attachment 8625162 [details] [diff] [review]
Mark probably-invalid webref test as failing. v1

Review of attachment 8625162 [details] [diff] [review]:
-----------------------------------------------------------------

The test isn't wrong I don't think. 
Now we may want to ignore it for now. 

Only data eviction is asynchronous. How we filter the buffered range upon call to endOfStream is another matter. 
What we need to do is query the source buffer buffered ranges and apply what the spec requires. 

TrackBuffer can use its own monitor to protect its cached buffered range rather than relying on the MediaSourceDecoder monitor. 

I'll write something up later

Comment 76

[Jean-Yves Avenard [:jya]]

Comment on attachment 8625161 [details] [diff] [review]
Use mirroring for buffered ranges. r=jya

Review of attachment 8625161 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +1307,5 @@
> +  if (HasAudio()) {
> +    UpdateReceivedNewData(TrackType::kAudioTrack);
> +  }
> +  if (HasVideo()) {
> +    videoti = mVideo.mTimeRanges;

this can be moved up now and merge with the previous HasVideo()

@@ +1310,5 @@
> +  if (HasVideo()) {
> +    videoti = mVideo.mTimeRanges;
> +  }
> +  if (HasAudio()) {
> +    audioti = mAudio.mTimeRanges;

Same for this one

Comment 77

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=167d7a151f0b

Comment 78

[Jean-Yves Avenard [:jya]]

Attachment: Returns proper time ranges when endOfStream was called.

Comment 79

[Jean-Yves Avenard [:jya]]

Attachment: Make it work with about:media plugin

To be merge with P6-4 patch

Comment 80

[Bobby Holley (:bholley)]

Comment on attachment 8625049 [details] [diff] [review]
P6-3. Cache SourceBuffer buffered ranges.

Review of attachment 8625049 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/TrackBuffer.cpp
@@ +262,5 @@
> +  ProxyMediaCall(mParentDecoder->GetReader()->TaskQueue(), this, __func__,
> +                 &TrackBuffer::UpdateBufferedRanges,
> +                 mLastAppendRange, /* aNotifyParent */ true)
> +      ->Then(mParentDecoder->GetReader()->TaskQueue(), __func__,
> +             [self] (bool) {

Don't need to declare these args, here and below.

@@ +288,5 @@
> +}
> +
> +nsRefPtr<TrackBuffer::BufferedRangesUpdatedPromise>
> +TrackBuffer::UpdateBufferedRanges(Interval<int64_t> aByteRange, bool aNotifyParent)
> +{

This should assert OnTaskQueue.

@@ +884,5 @@
> +  ProxyMediaCall(mParentDecoder->GetReader()->TaskQueue(), this, __func__,
> +                 &TrackBuffer::UpdateBufferedRanges,
> +                 Interval<int64_t>(), /* aNotifyParent */ true)
> +      ->Then(mParentDecoder->GetReader()->TaskQueue(), __func__,
> +             [self] (bool) {

Here and below as well.

::: dom/media/mediasource/TrackBuffer.h
@@ +24,5 @@
>  class MediaSourceDecoder;
>  class MediaByteBuffer;
>  
> +using media::TimeIntervals;
> +using media::Interval;

In general, we're not supposed to use |using| statements in header files, but this is probably fine.

Comment 81

[Bobby Holley (:bholley)]

Comment on attachment 8625050 [details] [diff] [review]
P6-4. Cache SourceBuffer buffered ranges.

Review of attachment 8625050 [details] [diff] [review]:
-----------------------------------------------------------------

Apparently most of this goes away in the next patch, so...

Comment 82

[Bobby Holley (:bholley)]

Comment on attachment 8625398 [details] [diff] [review]
Returns proper time ranges when endOfStream was called.

Review of attachment 8625398 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/MediaSourceDecoder.cpp
@@ +103,5 @@
> +MediaSourceDecoder::GetBuffered()
> +{
> +  if (!NS_IsMainThread()) {
> +    return MediaDecoder::GetBuffered();
> +  }

Please assert against this case.

Comment 83

[Bobby Holley (:bholley)]

Comment on attachment 8625399 [details] [diff] [review]
Make it work with about:media plugin

Review of attachment 8625399 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 84

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e06368496d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2cefa397dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ffc9a9ac48
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bd7a0d2aea
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa5fa1d318e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6776ce74b9e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02dd312d622
https://hg.mozilla.org/integration/mozilla-inbound/rev/a369cfb95b59

Comment 85

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent media test failures, mostly e10s.
https://treeherder.mozilla.org/logviewer.html#?job_id=11029470&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11028211&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11029938&repo=mozilla-inbound

etc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/85029878321b

Comment 86

[Jean-Yves Avenard [:jya]]

bholley Id be very keen to split this bug in three parts (SafeReadAt and the changes to MediaSourceDecoder)
If this one can't be landed quickly

Comment 87

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #86)
> bholley Id be very keen to split this bug in three parts (SafeReadAt and the
> changes to MediaSourceDecoder)
> If this one can't be landed quickly

I'm closing in on the last bug. Hope to get there today.

Comment 88

[Bobby Holley (:bholley)]

Attachment: Implement SilentReadAt - Adendum. v1

http://sadtrombone.vorb.is/

Comment 89

[Jean-Yves Avenard [:jya]]

Comment on attachment 8626825 [details] [diff] [review]
Implement SilentReadAt - Adendum. v1

Review of attachment 8626825 [details] [diff] [review]:
-----------------------------------------------------------------

Doh!

Comment 90

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43111497d4a1

Comment 91

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a894ed5740
https://hg.mozilla.org/integration/mozilla-inbound/rev/b043b3ec9ccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0810020721e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a10b4a2d1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/36d06eceb29d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d17eedad8a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b56f26141b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb9dbd7a0dd

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40a894ed5740
https://hg.mozilla.org/mozilla-central/rev/b043b3ec9ccb
https://hg.mozilla.org/mozilla-central/rev/0810020721e7
https://hg.mozilla.org/mozilla-central/rev/94a10b4a2d1e
https://hg.mozilla.org/mozilla-central/rev/36d06eceb29d
https://hg.mozilla.org/mozilla-central/rev/0d17eedad8a9
https://hg.mozilla.org/mozilla-central/rev/f5b56f26141b
https://hg.mozilla.org/mozilla-central/rev/9fb9dbd7a0dd

Comment 93

[Marco Castelluccio [:marco]]

This or bug 1177501 caused bug 1180214.

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This was apparently partially reverted in https://hg.mozilla.org/mozilla-central/rev/ac46c38d047f