This is required to use the new TrackBuffersManager introduced in bug 1119208

Add MediaDataDemuxer::IsThreadSafe() method. The new media source demuxer can't support cloning. Cloning() works if you can share the MediaResource object, which in the case of media source, we don't have

Handle EOS and WAITING_FOR_DATA during skip to next key frame. This is what caused occasional stalls in the facebook MSE video, or the video not playing to the end.

The MediaSourceDemuxer object.

Rebase. Change commit comment

Make media source reference time be 0 as per spec. Remove redundant virtual keyword when override is specified.

(In reply to Jean-Yves Avenard [:jya] from comment #1) > Created attachment 8615676 [details] [diff] [review] > P1. Add MediaDataDemuxer::IsThreadSafe() method > > Add MediaDataDemuxer::IsThreadSafe() method. The new media source demuxer > can't support cloning. Cloning() works if you can share the MediaResource > object, which in the case of media source, we don't have This is only required due to how GetBuffered() is working. Bobby is working on removing the multi-thread access and using the state mirroring mechanism. Once this is done, we will be able to remove completely the demuxer cloning (there can be only one!)

Rebase

Rebase

rebase

rebase

Force update of buffered range cache when calling endofstream

Use ProxyMediaCall and remove need for monitor.

Ensure all source buffers are ready before resolving ReadMetadata(). Only recalculate the next keyframe time when needed

Ensure all source buffers are ready before resolving ReadMetadata(). Only recalculate the next keyframe time when needed

Only clear decoder.mInputExhausted once we feed data to the decoder. This ensure that we never stop attempting to decode while the decoder is awaiting for content. The flag was cleared as we were attempting to demux, demux which could have failed.

Only allow seeking if we have the target data. We make an exception when seeking to 0, as it really means 'to the first frame' and a video may not exactly starts at 0.

Comment on attachment 8617148 [details] [diff] [review] P3. Add MediaSourceDemuxer object Review of attachment 8617148 [details] [diff] [review]: ----------------------------------------------------------------- Please replace your use of "auto" with the actual types, so that the code is easier to read and understand. Code is read more than its written, so optimize for readability. "auto" is ok when it's not important what the type is, like for iterators, or it's obvious. ::: dom/media/mediasource/MediaSourceDecoder.h @@ +111,5 @@ > // calls Attach/DetachMediaSource on this decoder to set and clear > // mMediaSource. > dom::MediaSource* mMediaSource; > + nsRefPtr<MediaDecoderReader> mReader; > + bool mFormatReader; Please call this mIsUsingFormatReader, or mHasFormatReader, or some other "mIs..." or "mHas..." pattern, so it's obvious from the variable name that this is a boolean. ::: dom/media/mediasource/MediaSourceDemuxer.cpp @@ +55,5 @@ > +{ > + mMonitor.AssertCurrentThreadOwns(); > + > + // Reset our current content. > +// mInfo = MediaInfo(); You shouldn't be checking in commented out code. And if you were, the indent is wrong. @@ +297,5 @@ > + } > + const auto& track = mManager->GetTrackBuffer(mType); > + TimeUnit lastKeyFrameTime; > + uint32_t lastKeyFrameIndex = 0; > + for (uint32_t i = 0; i < track.Length(); i++) { Is it worth using (or at least considering using) a bisection search here instead? nsTArray has bisection search in-built. @@ +352,5 @@ > + int32_t parsed = 0; > + const auto& track = mManager->GetTrackBuffer(mType); > + for (uint32_t i = mNextSampleIndex; i < track.Length(); i++) { > + const auto& sample = track[i]; > + if (sample->mKeyframe && sample->mTime >= aTimeThreadshold.ToMicroseconds()) { It's a shame that sample->mTime here is in microseconds rather than as a TimeUnit so you still need to explicitly convert the time units. But I can't actually tell what type |sample| is, and therefore what the type of mTime is, because you're using auto here. @@ +394,5 @@ > + return nullptr; > + } > + > + if (mNextSampleIndex) { > + auto& sample = track[mNextSampleIndex]; Because you're using auto here it's impossible to tell whether |sample| is a refcounted type. In general, you should only use auto where it's obvious what the type is, or the type is template-hell, like a std::iterator. ::: dom/media/mediasource/MediaSourceDemuxer.h @@ +25,5 @@ > +{ > +public: > + explicit MediaSourceDemuxer(); > + > + virtual nsRefPtr<InitPromise> Init() override; "Method declarations must use at most one of the following keywords: "virtual", "override", or "final"." https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style @@ +122,5 @@ > + media::TimeUnit mNextRandomAccessPoint; > + MediaPromiseHolder<SeekPromise> mSeekPromise; > + MediaPromiseHolder<SamplesPromise> mSamplePromise; > + MediaPromiseHolder<SkipAccessPointPromise> mSkipPromise; > + Atomic<bool> mDetached; Does this really need to be atomic?

(In reply to Chris Pearce (:cpearce) from comment #17) > Comment on attachment 8617148 [details] [diff] [review] > P3. Add MediaSourceDemuxer object > > > ::: dom/media/mediasource/MediaSourceDemuxer.cpp > @@ +55,5 @@ > > +{ > > + mMonitor.AssertCurrentThreadOwns(); > > + > > + // Reset our current content. > > +// mInfo = MediaInfo(); > > You shouldn't be checking in commented out code. And if you were, the indent > is wrong. this is fixed in patch 8. > > @@ +297,5 @@ > > + } > > + const auto& track = mManager->GetTrackBuffer(mType); > > + TimeUnit lastKeyFrameTime; > > + uint32_t lastKeyFrameIndex = 0; > > + for (uint32_t i = 0; i < track.Length(); i++) { > > Is it worth using (or at least considering using) a bisection search here > instead? nsTArray has bisection search in-built. > it could be used yes... by using a reference though, we avoid creating unnecessary copy or new refptr. > It's a shame that sample->mTime here is in microseconds rather than as a > TimeUnit so you still need to explicitly convert the time units. need to change the MediaData object, it's on my TODO list. > > But I can't actually tell what type |sample| is, and therefore what the type > of mTime is, because you're using auto here. > > @@ +394,5 @@ > > + return nullptr; > > + } > > + > > + if (mNextSampleIndex) { > > + auto& sample = track[mNextSampleIndex]; > > Because you're using auto here it's impossible to tell whether |sample| is a > refcounted type. it's a reference, so it really doesn't matter if it's refcounted or not... I guess we can't tell it's a pointer though. will change. > > In general, you should only use auto where it's obvious what the type is, or > the type is template-hell, like a std::iterator. > > ::: dom/media/mediasource/MediaSourceDemuxer.h > @@ +25,5 @@ > > +{ > > +public: > > + explicit MediaSourceDemuxer(); > > + > > + virtual nsRefPtr<InitPromise> Init() override; > > "Method declarations must use at most one of the following keywords: > "virtual", "override", or "final"." > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style > yeah, that is fixed in a later patch... will shift the changes and rebase. > @@ +122,5 @@ > > + media::TimeUnit mNextRandomAccessPoint; > > + MediaPromiseHolder<SeekPromise> mSeekPromise; > > + MediaPromiseHolder<SamplesPromise> mSamplePromise; > > + MediaPromiseHolder<SkipAccessPointPromise> mSkipPromise; > > + Atomic<bool> mDetached; > > Does this really need to be atomic? well, it was accessed from two different threads, and it's easier to deal with atomics than monitors. having said that, all of this part is gone in a later patch.

(In reply to Chris Pearce (:cpearce) from comment #17) > Comment on attachment 8617148 [details] [diff] [review] > P3. Add MediaSourceDemuxer object > > Review of attachment 8617148 [details] [diff] [review]: > Is it worth using (or at least considering using) a bisection search here > instead? nsTArray has bisection search in-built. I'll take this in bug 1171760.

Rebasing. Addressed comments. Carrying r+

Rebasing due to changes in P3

remove now useless variables.

Rebase.

Rebase.

Rebase

Comment on attachment 8617194 [details] [diff] [review] P8. Ensure we will always decode available data Review of attachment 8617194 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaFormatReader.cpp @@ +875,5 @@ > } > decoder.mQueuedSamples.Clear(); > + > + // We have serviced the decoder's request for more data. > + decoder.mInputExhausted = false; Does this fix a bug inherited from MP4Reader? I'm wondering if there's a bug there on Aurora/Beta that we could fix.

Comment on attachment 8617792 [details] [diff] [review] P4. Don't compute start time for MSE Review of attachment 8617792 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDataDemuxer.h @@ +101,5 @@ > > + // Indicate to MediaFormatReader if it should compute the start time > + // of the demuxed data. If true (default) the first sample returned will be > + // used as reference time base. > + virtual bool ComputeStartTime() const { return true; } You have two functions, MediaDataDemuxer::ComputeStartTime() and MediaFormatReader::ComputeStartTime() and they do different things but have the same name. Please call this "MustComputeStartTime()" or something like that which differentiates that this function does not compute the start time, but in fact tells you that you *need* to compute the start time.

(In reply to Chris Pearce (:cpearce) from comment #27) > Comment on attachment 8617792 [details] [diff] [review] > P4. Don't compute start time for MSE And it would have been easier to review if you did the virtual/override changes in a separate patch.

(In reply to Chris Pearce (:cpearce) from comment #28) (In reply to Chris Pearce (:cpearce) from comment #26) > Comment on attachment 8617194 [details] [diff] [review] > P8. Ensure we will always decode available data > > Review of attachment 8617194 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaFormatReader.cpp > @@ +875,5 @@ > > } > > decoder.mQueuedSamples.Clear(); > > + > > + // We have serviced the decoder's request for more data. > > + decoder.mInputExhausted = false; > > Does this fix a bug inherited from MP4Reader? I'm wondering if there's a bug > there on Aurora/Beta that we could fix. Hmmm... indeed, MP4Reader would have the same issue. Though the call to GetSample() is blocking and less likely to fail (as MediaSourceReader only ask for data if it *knows* the reader has the data.

Comment on attachment 8617808 [details] [diff] [review] P7. Ensure we have all sourcebuffer ready Review of attachment 8617808 [details] [diff] [review]: ----------------------------------------------------------------- r+, but I'd like an answer on my question/comment regarding crypto being overwritten. ::: dom/media/mediasource/MediaSourceDemuxer.cpp @@ +56,3 @@ > MonitorAutoLock mon(mMonitor); > > + bool haveSourceBufferNotReady = false; Maybe call that haveEmptySourceBuffer, so that you don't have to return a double negative below to improve understandability. @@ +69,5 @@ > mInfo.mVideo = info.mVideo; > mVideoTrack = sourceBuffer; > } > if (info.IsEncrypted() && !mInfo.IsEncrypted()) { > mInfo.mCrypto = info.mCrypto; Does this line "mInfo.mCrypto = info.mCrypto" mean if we have two source buffers with different crypto info, we'll overwrite the crypto info from the first one we encounter with the latter? Isn't that wrong?

(In reply to Chris Pearce (:cpearce) from comment #30) > Does this line "mInfo.mCrypto = info.mCrypto" mean if we have two source > buffers with different crypto info, we'll overwrite the crypto info from the > first one we encounter with the latter? Isn't that wrong? Actually, we have the problem that this value is only read during MediaDecoderReader::ReadMetadata(). With the existing MSE code, we have multiple sub-readers. One per init segment. So we always get one call to ReadMetadata() per crypto entry. But with the new MSE and its single vector of samples, we have no way to pass the updated crypto back to the MediaFormatReader() each time we have an appendBuffer with new encrypted data. So all we want to do here, is tell the MediaFormatReader() that there's encrypted data to worry about, regardless of which source buffer. The handling of encrypted/key event would instead be done at the TrackBuffersManager level which will directly call MediaSourceDecoder and let it fire the encrypted event each time a new init segment is read. When a new init segment is appended to the source buffer. If the future data is encrypted, then DispatchEncrypted() will be called on the MediaSourceDecoder's owner and MediaFormatReader doesn't have to worry about it any longer (but we do want it to create a CDM Wrapper, hence why we tell it mInfo is encrypted) I'll have that in another bug.

Add GetSamplesMayBlock() and implement MediaFormatReader::UseBufferingHeuristics()

We need to perform our fuzzy search (the timeintervals has a fuzz that is half the frame duration) on the start of the interval only and always exclude the end

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f187a37e3e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6e7468b0e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b05400738d https://hg.mozilla.org/integration/mozilla-inbound/rev/b5915eff97e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/34d1dedc56c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/13019071a4ab https://hg.mozilla.org/integration/mozilla-inbound/rev/0628a6c3be52 https://hg.mozilla.org/integration/mozilla-inbound/rev/29a865594872 https://hg.mozilla.org/integration/mozilla-inbound/rev/2328d14a175c https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7919145d65 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2657655cde

https://hg.mozilla.org/mozilla-central/rev/3f187a37e3e9 https://hg.mozilla.org/mozilla-central/rev/3a6e7468b0e0 https://hg.mozilla.org/mozilla-central/rev/e2b05400738d https://hg.mozilla.org/mozilla-central/rev/b5915eff97e5 https://hg.mozilla.org/mozilla-central/rev/34d1dedc56c4 https://hg.mozilla.org/mozilla-central/rev/13019071a4ab https://hg.mozilla.org/mozilla-central/rev/0628a6c3be52 https://hg.mozilla.org/mozilla-central/rev/29a865594872 https://hg.mozilla.org/mozilla-central/rev/2328d14a175c https://hg.mozilla.org/mozilla-central/rev/ea7919145d65 https://hg.mozilla.org/mozilla-central/rev/ff2657655cde

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

wrong bug #