Closed Bug 1251460 Opened 9 years ago Closed 9 years ago

buffered attribute may be empty even after loadeddata event has been fired

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jya, Assigned: bryce)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded
58 bytes, text/x-review-board-request
jya
: review+
Details
Seen while looking into bug 657791. When loadeddata is fired, it indicates that readyState is >= HAVE_FUTURE_DATA Yet, the update buffered attribute is typically empty. The reason for this is that MediaDecoderReader::UpdateBuffered() is only called once MediaDecoderReader::NotifyDataArrived is called which may be well after the first frame has been decoded. We need to ensure that the buffered range has been updated before MediaDecoder::FirstFrameLoaded is called.
I am thinking about if we can use promise chaining to ensure events are fired in order as we did with StartTimeRendezvous in MDSM. It would be better for code maintenance to put all logic concerning event orders in one place.
I was thinking of having a MediaDecoderReader::UpdateBufferedWithPromise (or AyncUpdateBuffer to follow the existing trend) And the MDSM would call and chain to the firsframeloaded once resolved.
Priority: -- → P2
MediaDecoderStateMachine's EnqueueFIrstFrameLoadedEvent would previously call into MediaDecoderReader to update MDR's buffered ranges and enqueue the frame loaded event. This commit aims to instead have the buffered update take place, and only afterwards enqueue the event. This should remove the possibility that the event will be fired and handled before the update of the buffered ranges has taken place. Review commit: https://reviewboard.mozilla.org/r/37679/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37679/
Attachment #8725888 - Flags: review?(jyavenard)
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/1-2/
https://reviewboard.mozilla.org/r/37679/#review34229 ::: dom/media/MediaDecoderReader.h:239 (Diff revision 2) > + NotifyDataArrived(); Add some asssertions to ensure we really have some data to update buffer ranges. ::: dom/media/MediaDecoderStateMachine.cpp:2039 (Diff revision 2) > + ->Then(OwnerThread(), You need to use a MozPromiseRequestHolder to hold the result so that you can disconnect the request during shutdown.
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded https://reviewboard.mozilla.org/r/37679/#review34243 Following what JW wrote, you'll need to disconnect the promise holder in MDSM::Shutdown() ::: dom/media/MediaDecoderReader.h:239 (Diff revision 2) > + NotifyDataArrived(); Please comment on why NotifyDataArrived() is being used rather than a simple call to UpdateBuffered(). It will be better to make UpdateBufferedWithPromise() a virtual and have the default implementation call UpdateBuffered() like you had before (as that's all is needed there) And have a specialised version for the MediaFormatReader which instead needs to call NotifyDataArrived() The need to call NotifyDataArrived being unique to the MediaFormatReader as it cache the buffered range (this is for historical reasons on when reading the buffered attribute before, would cause to always recalculate everything).
Attachment #8725888 - Flags: review?(jyavenard)
https://reviewboard.mozilla.org/r/37679/#review34229 > Add some asssertions to ensure we really have some data to update buffer ranges. Are there any particular asserts that you had in mind? Following the update we could make sure that a range is present, but I'm uncertain as to what to check before the update to make sure new data is present.
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/2-3/
Attachment #8725888 - Flags: review?(jyavenard)
https://reviewboard.mozilla.org/r/37679/#review34229 > Are there any particular asserts that you had in mind? Following the update we could make sure that a range is present, but I'm uncertain as to what to check before the update to make sure new data is present. Like assert GetBuffered() never returns an empty set of time intervals. It would be wrong to resolve the promise when our buffer ranges are still empty, right?
https://reviewboard.mozilla.org/r/37679/#review34307 ::: dom/media/MediaDecoderStateMachine.cpp:2049 (Diff revision 2) > + mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(mInfo)), I am worried about event disordres when we don't always chain the notification to the promise. Later events might reach Mediadecoder before early events. It is also confusing and inconsistent to me that we don't wait for the promsie when mSentFirstFrameLoadedEvent is true. We should ask the reader if buffer ranges are present, we can send notifications immediately without waiting for the promise to be resolved. However, I don't think such a optimization really makes a difference since EnqueueFirstFrameLoadedEvent() is never the hot spot.
I don't see how you could ever enter dormant and resume from dormant prior the first frame being decoded and loadeddata fired.
https://reviewboard.mozilla.org/r/37679/#review34307 > I am worried about event disordres when we don't always chain the notification to the promise. Later events might reach Mediadecoder before early events. It is also confusing and inconsistent to me that we don't wait for the promsie when mSentFirstFrameLoadedEvent is true. We should ask the reader if buffer ranges are present, we can send notifications immediately without waiting for the promise to be resolved. However, I don't think such a optimization really makes a difference since EnqueueFirstFrameLoadedEvent() is never the hot spot. I'm not sure I entirely understand all the orderings that could happen here. What would be appropriate to handle the cases discussed at the start of this issue? Having the mSentFirstFrameLoadedEvent == true case not notify if the request still exists? Is there a way to query the reader for if buffered is populated, or would we have to update the interface to expose that information?
(In reply to Jean-Yves Avenard [:jya] from comment #15) > I don't see how you could ever enter dormant and resume from dormant prior > the first frame being decoded and loadeddata fired. No, it won't happen for now. But I want to make it easier when reasoning about the code without taking too much detail/context into account. It also makes the code easier to understand.
can we take this as a follow up bug then? because I'm fairly sure Bryce is dying to get bug 657791 landed; and he needs this bug to proceed.
https://reviewboard.mozilla.org/r/37679/#review34307 > I'm not sure I entirely understand all the orderings that could happen here. What would be appropriate to handle the cases discussed at the start of this issue? Having the mSentFirstFrameLoadedEvent == true case not notify if the request still exists? > > Is there a way to query the reader for if buffered is populated, or would we have to update the interface to expose that information? 1. You can send the notification in the lambda passed to Then(). It is the merit of promise that your callback function will always be executed no matter the promise is already resolved or will be resolved later. 2. You need update the interface. However, it will involve some cross-thread reading which we would like to avoid. Therefore I said the optimization is not worth it.
(In reply to Jean-Yves Avenard [:jya] from comment #18) > can we take this as a follow up bug then? > > because I'm fairly sure Bryce is dying to get bug 657791 landed; and he > needs this bug to proceed. OK. I am fine with that.
https://reviewboard.mozilla.org/r/37679/#review34307 > 1. You can send the notification in the lambda passed to Then(). It is the merit of promise that your callback function will always be executed no matter the promise is already resolved or will be resolved later. > 2. You need update the interface. However, it will involve some cross-thread reading which we would like to avoid. Therefore I said the optimization is not worth it. 1. Forgive my lack of knowledge of promises, are there examples of how I would do this?
RefPtr<MediaDecoderStateMachine> self = this; mBufferedUpdateRequest.Begin(InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, &MediaDecoderReader::UpdateBufferedWithPromise) ->Then(OwnerThread(), __func__, // Resolve [self]() { self->mBufferedUpdateRequest.Complete(); MediaDecoderEventVisibility visibility = self->mSentFirstFrameLoadedEvent ? MediaDecoderEventVisibility::Suppressed : MediaDecoderEventVisibility::Observable; self->mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)), visibility); self->mSentFirstFrameLoadedEvent = true; }, // Reject []() { MOZ_ASSERT(false, "Should not reach"); })); Always call mFirstFrameLoadedEvent.Notify() in the resolve function. So if there are multiple calls to EnqueueFirstFrameLoadedEvent(), mFirstFrameLoadedEvent.Notify() in later calls will never execute before the one in the 1st call.
fairly sure that setting mSentFirstFrameLoadedEvent asynchronously will break things as the code expect it to be true when returning from that function (and in particular could cause further decoding than necessary)
https://reviewboard.mozilla.org/r/37679/#review34307 > 1. Forgive my lack of knowledge of promises, are there examples of how I would do this? I may have misunderstood the above, are you suggesting the entire if else be inside the then? My original reading was that in the else I could somehow update the lambda from the else.
so instead you could set it to true but pass the original value to the lambda. that will work, and avoid the issue of the function being called out of order.
IMHO this is what's required: RefPtr<MediaDecoderStateMachine> self = this; bool firstFrameLoaded = mSentFirstFrameLoadedEvent; mSentFirstFrameLoadedEvent = true; mBufferedUpdateRequest.Begin(InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, &MediaDecoderReader::UpdateBufferedWithPromise) ->Then(OwnerThread(), __func__, // Resolve [firstFrameLoaded, self]() { self->mBufferedUpdateRequest.Complete(); MediaDecoderEventVisibility visibility = firstFrameLoaded ? MediaDecoderEventVisibility::Suppressed : MediaDecoderEventVisibility::Observable; self->mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)), visibility); }, // Reject []() { MOZ_CRASH("Should not reach"); }));
I was considering whether to update mSentFirstFrameLoadedEvent inside the resolve function or beforehand. It looks like the correct one is to update it beforehand.
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/3-4/
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded https://reviewboard.mozilla.org/r/37679/#review34335 r=me with nit addressed. ::: dom/media/MediaDecoderReader.h:240 (Diff revision 4) > + virtual RefPtr<BufferedUpdatePromise> UpdateBufferedWithPromise() { { on new line ::: dom/media/MediaDecoderReader.h:243 (Diff revision 4) > + MOZ_ASSERT(mBuffered.Ref().Length() > 0); I would place that assert in MDSM upon resolution of the promise, that way it will be tested for both cases without duplicating code. The MDSM mirror mBuffered so you can use it there safely. And use !IsEmpty() ::: dom/media/MediaDecoderStateMachine.cpp:2052 (Diff revision 4) > + []() { MOZ_ASSERT(false, "Should not reach"); })); MOZ_CRASH("...") ::: dom/media/MediaFormatReader.h:41 (Diff revision 4) > + RefPtr<BufferedUpdatePromise> UpdateBufferedWithPromise() override; Could you place it alongside GetBuffered() seeing that they are related.
Attachment #8725888 - Flags: review?(jyavenard) → review+
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/4-5/
https://reviewboard.mozilla.org/r/37679/#review34335 > I would place that assert in MDSM upon resolution of the promise, that way it will be tested for both cases without duplicating code. > > The MDSM mirror mBuffered so you can use it there safely. > > And use !IsEmpty() I've made the chanegs noted except !IsEmpty -- I don't believe interval sets expose that convenience method, so without setting it I have to settle for Lenght().
Looking at the results of the try, I'm concerned that: 1) I've introduced regressions. I don't like the number of tests timing out in dom/media. 2) The new asserts are being hit. Meaning there are still times when there's not a buffered range after the new promise is resolved. Investigating now.
I can see how that could be happening. IRC, If a read is less than a particular size, the data isn't cached and as such the buffered range will be empty. I'd be fine with the removal of the assert. The other timeouts where the assert didn't occur is a bit of a worry though
(In reply to Bryce Van Dyk (:SingingTree) from comment #36) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc29a76e111 Trying without the assert ^
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/5-6/
https://reviewboard.mozilla.org/r/37679/#review35215 ::: dom/media/MediaDecoderStateMachine.cpp:1352 (Diff revisions 5 - 6) > + // times we don't want to lose the promise, such as when seeking IMHO, you can remove that comment. as disconnecting on shutdown was always the place where it needed to be done ::: dom/media/MediaFormatReader.cpp:1625 (Diff revisions 5 - 6) > + // readers as some do not support buffered ranges (chained oggs, for trailing space. the comment seems misplaced to me, seeing that here you are explaining why the assert wasn't put in the MDSM; yet the context of your change will be lost and it will make no sense for the future reader seeing that MediaFormatReader is currently completely unrelated to the OggReader. So only keep the first sentence, adding that UpdateBufferedwithPromise is called by the MDSM after decoding the first frame. And as such, data *must* be buffered at this stage, not should. To me must == MOZ_ASSERT -> fatal error and must crash should == NS_WARNING / NS_ASSERTION -> soft error ::: dom/media/test/manifest.js:476 (Diff revision 6) > - { name:"r11025_s16_c1.wav", type:"audio/x-wav", duration:1.0 }, > + // { name:"r11025_s16_c1.wav", type:"audio/x-wav", duration:1.0 }, don't think you intended to keep this change before you commit !
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/6-7/
Assignee: nobody → bvandyk
I'm seeing an error here with the assert in MediaFormatReader: > MOZ_ASSERT(mBuffered.Ref().Length() > 0); dom/media/test/test_bug448534.html exercises an issue, which I'm seeing specifically against the seek.webm test file. NotifyDataArrived() is called, during which MediaDecoderReader's GetBuffered() is called. This in turn calls UpdateReceivedNewData() on the appropriate track. However, I'm seeing inconsistent results in UpdateReceivedNewData, sometimes the decoder has a buffered range. Other times no buffered range is present on the decoder and the it has been drained. I'll keep looking into what's going on, but was wondering if you have any thoughts on what's going on here, jya?
From top of my hean, I have no logical explanation on the behaviour you describe https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#787 If MediaFormatReader::NotifyDataArrived has been called then decoder.mReceivedNewData will be true, and MediaDataDemuxer::GetBuffered() will be called, this will force the demuxer to recalculate the buffered range. Now maybe it could be that the MediaCacheStream hasn't done any work yet, or hasn't updated the required bit (it runs on the main thread, but it would be bizarre as we've just read data from it). roc would know, but I'm guessing this is going to be a problem now. In the mean time, you can check to ensure that the demuxer's GetBuffered is properly recalculating the buffered range, and that their MediaResource::GetCachedRanges returns what's expected. Which file in this test is causing the problem?
I'm seeing it happen with seek.webm. It appears to be intermittent but I have a fairly high hit rate of the failure path: ~50%+ on my work laptop.
I believe the issue happening with the above test is that it's possible to create and add an HTMLMediaElement to a page and very quickly modify its source attribute (removeNodeAndSource). This results in a race between the async task updating the buffered state, and the shutdown. I believe the manifestation is the above, where by the time the buffered range is being updated, the underlying buffered range has already been torn down by the shutdown procedure, resulting in there being a 0 number of intervals and the assert failing.
well, remove the assert then :)
Commit incoming. I've been searching for if there's a solution that could check to see if a shutdown has started/been done, but I am unable to find an elegant way to do so without modifying other code, which seems OTT to enable this assert.
you could test if mShutdown is true and if so skip the assert. but IMHO it really doesn't matter. so may as well removing the assert
Depending on how the components race, it appears possible for the MediaFormatReader to no be shutdown yet, but to have its decoder shutdown. However, since the internal decoder is exposed via an AbstractMediaDecoder pointer, there's not a lot of inspection that can be done on it (at least regarding the shutdown) from what I can tell. Just removed the assert, as you say.
The previous changes introduce an assert in UpdateBufferedWithPromise which assumes that after updating there will always be some buffered interval. However, it appears possible to very quickly shutdown the underlying decoder and have it purge any buffered data and to race the UpdateBufferedWithPromise check. The test_bug448534.html test highlighted this. I think it's valid to have an empty buffered range in the case where a shutdown has happened, and I cannot find an elegant way to reword the assert, as such I'm removing it. Review commit: https://reviewboard.mozilla.org/r/38857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38857/
Attachment #8728200 - Flags: review?(jyavenard)
yeah, all MediaDecoder methods must be accessed on the main thread. Could always mirror MediaDecoder::mPlayState and test if it's PLAY_STATE_SHUTDOWN
Comment on attachment 8728200 [details] MozReview Request: Bug 1251460 - Remove assert introduced in previous commit. r?jya https://reviewboard.mozilla.org/r/38857/#review35517 please merge the two commits... this one is unecessary
Attachment #8728200 - Flags: review?(jyavenard) → review+
Comment on attachment 8725888 [details] MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37679/diff/7-8/
Attachment #8725888 - Attachment description: MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded r?jya → MozReview Request: Bug 1251460 - MDSM now waits on a promise to enqueue first frame loaded
Attachment #8728200 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/94aaebb92d0d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1320258
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: