Closed Bug 1175768 Opened 4 years ago Closed 4 years ago

Run NotifyDataArrived on the reader task queue and mirror buffered ranges

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Depends on 1 open bug)

Details

Attachments

(16 files, 21 obsolete files)

2.23 KB, patch
jya
: review+
Details | Diff | Splinter Review
11.65 KB, patch
jya
: review+
Details | Diff | Splinter Review
1.46 KB, patch
jya
: review+
Details | Diff | Splinter Review
3.88 KB, patch
Details | Diff | Splinter Review
7.09 KB, patch
jya
: review+
Details | Diff | Splinter Review
1.16 KB, patch
bholley
: review+
Details | Diff | Splinter Review
38.87 KB, patch
jya
: review+
Details | Diff | Splinter Review
12.68 KB, patch
bholley
: review+
Details | Diff | Splinter Review
25.69 KB, patch
bholley
: review+
Details | Diff | Splinter Review
22.59 KB, patch
Details | Diff | Splinter Review
38.58 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.05 KB, patch
jya
: review-
Details | Diff | Splinter Review
33.97 KB, application/zip
Details
4.49 KB, patch
bholley
: review+
Details | Diff | Splinter Review
21.69 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.67 KB, patch
jya
: review+
Details | Diff | Splinter Review
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.
With this change, we don't need to condition this work on infinite streams.
NotifyDataArrived will soon run off-main-thread, so the assumptions here won't hold.
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.
This currently asserts for the MSE case.
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
Flags: needinfo?(jyavenard)
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 :)
Attachment #8623992 - Flags: review+
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.
Flags: needinfo?(jyavenard)
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 ???
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.
Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.
Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.
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 :)
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.
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
This probably isn't the right thing, but I needed this to avoid null derefs.
Attachment #8624386 - Flags: feedback?(jyavenard)
Attached patch Misc Fixes. v2 (obsolete) — Splinter Review
Attachment #8624386 - Attachment is obsolete: true
Attachment #8624386 - Flags: feedback?(jyavenard)
Attachment #8624494 - Flags: feedback?(jyavenard)
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 ??
Attachment #8624494 - Flags: feedback?(jyavenard) → feedback+
Attachment #8624385 - Attachment is obsolete: true
Attachment #8624560 - Attachment description: Use mirroring for buffered ranges. P6-1 v2 → Part 6-1 - Use mirroring for buffered ranges. P6-1 v2
This fixes the test_playback_rate failure.
Attachment #8623990 - Attachment is obsolete: true
Attached patch Misc Fixes. v3 (obsolete) — Splinter Review
This includes an 'explicit' to make the static analysis happy.
Attachment #8624494 - Attachment is obsolete: true
Attached patch P6-5. Misc Fix #2 (obsolete) — Splinter Review
This should hopefully fix the b2g failures.
Attachment #8624384 - Attachment is obsolete: true
Attachment #8623988 - Flags: review?(jyavenard)
Attachment #8623989 - Flags: review?(jyavenard)
Attachment #8624565 - Flags: review?(jyavenard)
Attachment #8624571 - Flags: review?(jyavenard)
Attachment #8624560 - Flags: review?(jyavenard)
Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.
Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.
Attachment #8624109 - Attachment is obsolete: true
Attachment #8624110 - Attachment is obsolete: true
Attachment #8624566 - Attachment is obsolete: true
Comment on attachment 8624569 [details] [diff] [review]
P6-5. Misc Fix #2

folded across P6-3 and P6-4
Attachment #8624569 - Attachment is obsolete: true
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.
Attachment #8623988 - Flags: review?(jyavenard) → review-
Comment on attachment 8623989 [details] [diff] [review]
Part 2 - Dispatch UpdateEstimatedMediaDuration. v1

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

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

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

Of course \o/ meant r+
Attachment #8623989 - Flags: review?(jyavenard) → review+
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 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
Attachment #8624565 - Flags: review?(jyavenard) → review+
Attachment #8624571 - Flags: review?(jyavenard) → review+
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.
Attachment #8624108 - Attachment is obsolete: true
Comment on attachment 8624583 [details] [diff] [review]
. P6-2 Remove now unused members.

actually no, it won't help...
Attachment #8624583 - Attachment is obsolete: true
Attachment #8624108 - Attachment is obsolete: false
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.
Attachment #8623988 - Flags: review- → review?(jyavenard)
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...
Attachment #8623988 - Flags: review?(jyavenard) → review+
(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*.
Attached file patches.zip (obsolete) —
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?
Flags: needinfo?(jyavenard)
(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.
(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.
Attached patch Throttle NotifyDataArrived. v1 (obsolete) — Splinter Review
This reduces the time spent in GetBuffered on the channel9 video from ~40% to
~1.5% on my machine.
Attachment #8625008 - Flags: review?(jyavenard)
(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 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
Attachment #8625008 - Flags: review?(jyavenard) → review+
(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.
Flags: needinfo?(jyavenard)
(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()
(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.
Flags: needinfo?(jyavenard)
Attached file patches.zip (obsolete) —
Attachment #8624862 - Attachment is obsolete: true
Attached file patches.zip (git) (obsolete) —
(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.
Attachment #8625008 - Attachment is obsolete: true
Attachment #8625020 - Flags: review+
(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)
Flags: needinfo?(jyavenard)
(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.
Flags: needinfo?(jyavenard)
Attached file patches.zip (git) (obsolete) —
Fully updated patches, including the throttling patch.
Attachment #8625016 - Attachment is obsolete: true
Attachment #8625019 - Attachment is obsolete: true
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?
Only resolve AppendPromise once we know all our sub-readers have recalculated their buffered ranges.
Handle sourcebuffer eviction. We can't call GetBuffered on the main thread. So we cache the buffered ranges within the track buffer.
Attachment #8624575 - Attachment is obsolete: true
Attachment #8624576 - Attachment is obsolete: true
Attachment #8625020 - Attachment is obsolete: true
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 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.
Attachment #8624560 - Flags: review?(jyavenard) → review+
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.
(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.
(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?
Attachment #8624560 - Attachment is obsolete: true
Attachment #8625161 - Flags: review+
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.
Attachment #8625161 - Flags: feedback?(jyavenard)
Attached file patches.zip (git)
Rebased patches.
Attachment #8625032 - Attachment is obsolete: true
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
Attachment #8625162 - Flags: review?(jyavenard) → review-
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
Attachment #8625161 - Flags: feedback?(jyavenard) → feedback+
To be merge with P6-4 patch
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.
Attachment #8625049 - Flags: review+
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...
Attachment #8625050 - Flags: review+
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.
Attachment #8625398 - Flags: review+
Comment on attachment 8625399 [details] [diff] [review]
Make it work with about:media plugin

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

rs=me
Attachment #8625399 - Flags: review+
Blocks: 1176852
Depends on: 1177282
Depends on: 1177501
Blocks: 1148102
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
(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 on attachment 8626825 [details] [diff] [review]
Implement SilentReadAt - Adendum. v1

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

Doh!
Attachment #8626825 - Flags: review?(jyavenard) → review+
Depends on: 1182287
Depends on: 1180403
This was apparently partially reverted in https://hg.mozilla.org/mozilla-central/rev/ac46c38d047f
See Also: → 1181641
Depends on: 1110922
Depends on: 1184429
Depends on: 1208953
Depends on: 1218157
You need to log in before you can comment on or make changes to this bug.