STR: - Open a page with an animated GIF - open a second tab and switch to it - wait a few seconds, move back to the tab with the animated GIF Result: the GIF will be played sped up for a while, related to the amount of time it remained in the background Expected: GIF will resume playing smoothly This appears to be a new regression as of 1 or max 2 weeks ago

Hmm, not sure that I can reproduce this. I tried nightly and http://imgur.com/gallery/uGsjT in a background tab. Can you give some more details for how to see this bug?

It's not clear to me if it happens on every gif, but you do need one with a lot of motion to be able to see it. See a recorded screencast that I got using this link: https://www.reddit.com/r/sports/comments/4k86aw/how_to_keep_a_runner_from_going_1st_to_3rd/

I can see the problem with that link, thanks! I'll look into it. Thanks for filing.

The gif is actually a video which explains the following: Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13e4cb81d4eb1c4996daf34c68301ca8a8ab0183&tochange=2fe467985f04512869ca7daa1a2db286642ce127 -> bug 1224973

Yes, we a suspending video decoders in background tabs to save on power usage. I'll take it.

[Tracking Requested - why for this release]: Regression

Tracking this visual regression for 49.

This feature is enabled for nightly only at this point.

Mass change P2 -> P3

* How to reproduce: So, the issue only happens to videos without audio channel and are set to be looping. And the effect could be reproduced by playing a video, switch it to background, wait for a moment until the video has reached the end and been restarted from the beginning again, then switch back to the foreground.

* The root cause: (1) For videos without audio channel, the playback is driven by the system clock and the system clock keeps going when the videos are switched to the background. (2) The media time is stopped when the video is switched to the background because no video frames are drawn from VideoSink, so the media time is not updated. (3) However, while switching back to foreground, we initiate a VideoOnlyAccurateSeek (or DecodeRecoverySeek) and set the target to the media time which was stopped. So, after the seek is resolved, the newly decoded video frames' time are much smaller than the system clock which prohibits the VideoSink to do scheduling but just keeps drawing the incoming video frames till the end of the video.

I have two proposals: * Proposal 1: While switching back to the foreground, use the system clock to calculate the right target (by mod duration). Stop the palyback for a while, invoke VideoOnlySeekOperation to the right target. Once the seek is resolved, start decoding and restart the playback. This could be a quick work around, however, there are two problems: (1) The MDSM must know weather the video is set to be looping. (2) Should we fire the ended event? and when? also the time-updated event.

* Proposal 2: In Bug 1224973, we stopped requesting video data when a video is switched to the background. Instead, I think we should still requesting video frames from the reader but let the reader know that do demuxing only (not decoding) and return NULL data. The MDSM still pushes the returned NULL data into video queue and the VideoSink keeps the playback without drawing the NULL data.

Personally, I think proposal 2 is better than 1 because it doesn't change data flow and keeps all the behaviors of MDSM, MediaDecoder and HTMLMediaElement. VideoSink is slightly modified to prevent drawing NULL data but it keeps all its playback behavior. MediaFormatReader is the where we will modify most, however the NULL data concept is already implemented in Bug 1257107 of which we might take advantage. Dan, WDYT? I think if we take proposal 2, it should also solve the "sending ended event" problem you faced while writing mochitests.

Why all the mess about looping? It happens with any video, e.g. attachment 8754969 [details]. I thought everybody already knew this.

(In reply to arni2033 from comment #15) > Why all the mess about looping? It happens with any video, e.g. attachment > 8754969 [details]. > I thought everybody already knew this. Yes, you are right, my fault. I was so focused on figuring out how to handle the looping cases so got confused and so did I write the wrong comment. Thanks for your reminding, this phenomenon is nothing about looping.

(In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #16) > Thanks for your reminding, this phenomenon is nothing about looping. But the solution needs to consider looping cases!

(In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #14) > Dan, WDYT? I think if we take proposal 2, it should also solve the "sending > ended event" problem you faced while writing mochitests. I didn't some thinking and prototyping of a solution for decoder shutdown. At the moment, we don't decode video and that saves CPU time but we don't free the decoder, so don't get saving on memory and freeing of, potential, H/W decoders. The prototype was very similar to Proposal 2. Instead of returning null frames, I tricked the MFR into releasing the video decoder it held and replaced it with the blank decoder. So, I think we should pursue Proposal 2.

(In reply to Dan Glastonbury :kamidphish from comment #18) > (In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #14) > > Dan, WDYT? I think if we take proposal 2, it should also solve the "sending > > ended event" problem you faced while writing mochitests. > > I didn't some thinking and prototyping of a solution for decoder shutdown. ^^^^^^^^^^^^^^^ Do you mean "I did have"? > The prototype was very similar to Proposal 2. Instead of returning null > frames, I tricked the MFR into releasing the video decoder it held and > replaced it with the blank decoder. I have discussed with JW and he think switching the video decode is a good idea, I think I could go on this way. Did you face any problems while prototyping?

(In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #19) > (In reply to Dan Glastonbury :kamidphish from comment #18) > > I didn't some thinking and prototyping of a solution for decoder shutdown. > ^^^^^^^^^^^^^^^ > Do you mean "I did have"? Yes, I meant to write "I did some thinking...". Sorry. > I have discussed with JW and he think switching the video decode is a good > idea, I think I could go on this way. Did you face any problems while > prototyping? The problem we face is resuming the decode without seeing a black (null decoder) or green (blank decoder) frame on resume. Otherwise, I hacked the MFR, it's inelegant and not the way to do it, but it was enough for testing. I'll forward you the patches.

Kaku, This is the hack that I made to switch to blank decoder. It's messy. The Seek() to next keyframe (In VisibilityChanged()) is not strictly necessary. I was attempting to speed up the recovery. We should speak with :jya on the best way to get decoder switching into MFR.

(In reply to Dan Glastonbury :kamidphish from comment #21) > Created attachment 8770338 [details] [diff] [review] > blank-decoder-hack.patch > > Kaku, This is the hack that I made to switch to blank decoder. It's messy. > The Seek() to next keyframe (In VisibilityChanged()) is not strictly > necessary. I was attempting to speed up the recovery. > > We should speak with :jya on the best way to get decoder switching into MFR. Thanks, Dan. I have a similar WIP implementation. I will upload it later and ask for :jya's comment.

Review commit: https://reviewboard.mozilla.org/r/64266/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64266/

Review commit: https://reviewboard.mozilla.org/r/64268/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64268/

(In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #24) > Created attachment 8770990 [details] > Bug 1274626 part 2 - [WIP] - switch the video decoder to blank decoder > dynamically; > > Review commit: https://reviewboard.mozilla.org/r/64268/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/64268/ @jya, Dan and I are now working on another version of suspending video decoding for non-visible video elements. Instead of stop requesting video data at the MDSM side (the implementation of bug 1224973), we would like to keep requesting video data but dynamically switch the video decoder to blank decoder. By this way, we make sure the playback logic is not affected so that the playback events are still filed as usual. This review request is a WIP patch and I would like to seek your feedback, especially on the "switching decoder part". Please have a look on this patch when you have time, thanks!

(In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #25) > (In reply to Tzuhao Kuo [:kaku] (PTO 6/30 ~ 7/3) from comment #24) > > Created attachment 8770990 [details] > > Bug 1274626 part 2 - [WIP] - switch the video decoder to blank decoder > > dynamically; > > > > Review commit: https://reviewboard.mozilla.org/r/64268/diff/#index_header > > See other reviews: https://reviewboard.mozilla.org/r/64268/ > > @jya, > > Dan and I are now working on another version of suspending video decoding > for non-visible video elements. Instead of stop requesting video data at the > MDSM side (the implementation of bug 1224973), we would like to keep > requesting video data but dynamically switch the video decoder to blank > decoder. By this way, we make sure the playback logic is not affected so > that the playback events are still filed as usual. > > This review request is a WIP patch and I would like to seek your feedback, > especially on the "switching decoder part". Please have a look on this patch > when you have time, thanks! I'm not entirely convinced it's going to be an easier approach. Because as far as the MediaFormatReader is concerned you're only going to add complexity over the existing code: You'll still need to seek in the video when resuming, recreate a decoder and handle catching up with the new video position. That means that the MDSM will have to deal with going backward in the decoding and handling of the now unnecessary future frames (as the MDSM is always about 10 video frames ahead of currentTime, those frames must be removed and redecoded with the real decoder)

(In reply to Jean-Yves Avenard [:jya] from comment #26) > I'm not entirely convinced it's going to be an easier approach. > Because as far as the MediaFormatReader is concerned you're only going to > add complexity over the existing code: Agree with that it adds complexity, however, I think it is a smoother approach. Previously, we stopped requesting video data from the MDSM side which causes problems: (1) "timeupdate" event might not be filed, (2) "ended" event is never be filed and (3) the side effect of this bug. The (1) happens with video files without audio track or video files whos video track is shorter than audio track. In these cases, the playback is driven by the system clock which never stops, however, the MDSM stops receiving video data from reader, it also stops filing "timeupdate" event because MDSM thinks that playback position is not possible to be where audio and video data haven't be decoded. The (2) happens with all video files. Since the MDSM stops requesting video data, the VideoQueue then never ends and the MDSM never files the "ended" event. Dan had a proposal to use "duration" to trigger "ended" event, but duration is not reliable which might changes while more video data is decoded, so JW didn't approve it. For (3), please refer to comment 11, comment 12 and comment 13. All (1), (2) and (3) will be solved together if we keep the video data flow (with only demuxing not real decoding) and that's why I think we should pursue the approach of switching decoder. @Dan, please correct me if any of the above description is wrong. > You'll still need to seek in the video when resuming, recreate a decoder and > handle catching up with the new video position. Even thought we do not change decoder, we still need to invoke a seek while resuming to catch up the audio's position. So, the extra effort is only recreating video decoder. > That means that the MDSM will have to deal with going backward in the > decoding and handling of the now unnecessary future frames (as the MDSM is > always about 10 video frames ahead of currentTime, those frames must be > removed and redecoded with the real decoder) The 10 video frames will be removed before the seek operation which resets the VideoQueue and they will be re-decoded because we seek to the currentTime which is in front of the 10 video frames.

@jya, does comment 27 make this approach more convincing?

if you think it will be easier that way.. then sure...

Review commit: https://reviewboard.mozilla.org/r/65544/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65544/

Review commit: https://reviewboard.mozilla.org/r/65546/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65546/

Review commit: https://reviewboard.mozilla.org/r/65548/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65548/

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64266/diff/1-2/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64268/diff/1-2/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/1-2/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/1-2/

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; https://reviewboard.mozilla.org/r/64266/#review62760 ::: dom/html/HTMLMediaElement.h:607 (Diff revision 2) > // data. Used for debugging purposes. > void GetMozDebugReaderData(nsAString& aString); > > void MozDumpDebugInfo(); > > + void SetVisible(bool aVisible); Let's call it MozSetVisibility and comment that this function is to simulate visibility changes to help debug and write tests about suspend-video-decoding. ::: dom/webidl/HTMLMediaElement.webidl:218 (Diff revision 2) > partial interface HTMLMediaElement { > [Throws, Pref="media.seekToNextFrame.enabled"] > Promise<void> seekToNextFrame(); > }; > + > +partial interface HTMLMediaElement { We need a DOM peer to review this. Please also hide this function behind a pref so it is never exposed to the public.

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; https://reviewboard.mozilla.org/r/65548/#review62762

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; https://reviewboard.mozilla.org/r/64268/#review63426 <p>I don't see the point of creating a NullDecodeModule (why not just reuse blankdecodermodule instead, at least it contains properly allocated data and is less likely to introduce regression)</p> ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:264 (Diff revision 2) > + aDTS.ToMicroseconds(), > + aDuration.ToMicroseconds(), > + VideoData::YCbCrBuffer(), > + true, > + aDTS.ToMicroseconds(), > + gfx::IntRect()); you'll want to use mInfo.ImageRect() as otherwise the HTMLMediaElement will consider the video frame as being invalid. ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:270 (Diff revision 2) > + } > +private: > + VideoInfo mInfo; > +}; > + > +class NullAudioDataCreator { When would you ever use a NullAudioDataDecoder? Any particular reason you're just not using the BlankDecoderModule instead? seems all this is unecessary complication. ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:285 (Diff revision 2) > + { > + // Convert duration to frames. We add 1 to duration to account for > + // rounding errors, so we get a consistent tone. > + CheckedInt64 frames = > + UsecsToFrames(aDuration.ToMicroseconds()+1, mSampleRate); > + if (!frames.isValid() || please put that into two lines instead ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:287 (Diff revision 2) > + // rounding errors, so we get a consistent tone. > + CheckedInt64 frames = > + UsecsToFrames(aDuration.ToMicroseconds()+1, mSampleRate); > + if (!frames.isValid() || > + !mChannelCount || > + !mSampleRate || There's no point testing the number of channel cound or sampling rate. Those are the original metadata values. The decoder is only ever created if those are valid

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; https://reviewboard.mozilla.org/r/65544/#review63424 This API is too complicated. Simply have a method to create an empty video decoder. Not having to set the platformdecoder module to null.Plus, this seems to configure the PDMFactory to create a null decoder module. Why? it can always be created when the PDMFactory is initialised the first time. So the use would now simply become: CreateDecoder with a parameter to always return a blank ::: dom/media/platforms/PDMFactory.h:47 (Diff revision 1) > // that we use on on aTaskQueue to decode the decrypted stream. > // This is called on the decode task queue. > void SetCDMProxy(CDMProxy* aProxy); > #endif > > + void SetNullDecode(); this is unecessary. Can always create the blank PDM in InitPDM() and store it separately like you've done here ::: dom/media/platforms/PDMFactory.cpp:53 (Diff revision 1) > > namespace mozilla { > > extern already_AddRefed<PlatformDecoderModule> CreateAgnosticDecoderModule(); > extern already_AddRefed<PlatformDecoderModule> CreateBlankDecoderModule(); > +extern already_AddRefed<PlatformDecoderModule> CreateNullDecoderModule(); I don't think we need this NullDecoder, the Blank one is better provided it returns valid frames. ::: dom/media/platforms/PlatformDecoderModule.h:39 (Diff revision 1) > static LazyLogModule sPDMLog("PlatformDecoderModule"); > > +enum class UseNullDecoder { > + USE_NULL_DECODER, > + NOT_USE_NULL_DECODER > +}; this is to complicated, and I don't see the advantage of an enum. Please use a bool

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; https://reviewboard.mozilla.org/r/65546/#review63428 ::: dom/media/MediaFormatReader.cpp:409 (Diff revision 2) > return false; > #endif > } > } > > + if (decoder.mIsNullDecode) { don't need this. Just call a new PDMFactory::CreateBlankVideoDecoder() ::: dom/media/MediaFormatReader.cpp:727 (Diff revision 2) > LOG("MediaFormatReader called DrainComplete() before flushing, ignoring."); > return; > } > decoder.mDrainComplete = true; > + > + if (decoder.mIsNullDecode) { why do so only when draining and only for the null decoder? ::: dom/media/MediaFormatReader.cpp:2066 (Diff revision 2) > + decoder.mIsNullDecode = aIsNullDecode; > + > + if (decoder.mIsNullDecode) { > + decoder.mNeedDraining = true; > + } else { > + // We are now using the null decoder, close it directly. shouldn't you finish the currently pending and already demuxed data, so that there's no discontinuity in the timestamp between the real decoded frames and the blank one?

https://reviewboard.mozilla.org/r/64268/#review63426 Jw and I thought that NullDecodeModule could even make VideoSink not draw the VideoData, slightly more efficient. But, you're right, it might be more error-prone and I should just use BlankDecoderModule. BTW, is it okay to change the default color of blank video decoder? It's now green which is uncomfortable (to me).

https://reviewboard.mozilla.org/r/65544/#review63424 Sure, this is simpler, thanks. > this is to complicated, and I don't see the advantage of an enum. > > Please use a bool I tried to prevent using a primitive type since we are not able to overload the CreateDecoderParams::Set() method. But, yes, this is over-optimized and we could delay this to the time the situation occurs.

https://reviewboard.mozilla.org/r/65546/#review63428 > shouldn't you finish the currently pending and already demuxed data, so that there's no discontinuity in the timestamp between the real decoded frames and the blank one? While switching from NullDecoder(or BlankDecoder) to normal decoder, a seek operations always follows, so the pending and already demuxed data will be removed. However, from your comments, I realized that while changing decoder, we should always drain and then shutdown the existing one so that we can create a new one, right?

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64266/diff/2-3/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64268/diff/2-3/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/1-2/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/2-3/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/2-3/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; https://reviewboard.mozilla.org/r/64268/#review63842 please update the commit comment. it is no longer relevant s/execpt/except ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:98 (Diff revision 3) > { > // Create a fake YUV buffer in a 420 format. That is, an 8bpp Y plane, > // with a U and V plane that are half the size of the Y plane, i.e 8 bit, > - // 2x2 subsampled. Have the data pointers of each frame point to the > - // first plane, they'll always be zero'd memory anyway. > - auto frame = MakeUnique<uint8_t[]>(mFrameWidth * mFrameHeight); > + // 2x2 subsampled. > + const int sizeY = mFrameWidth * mFrameHeight; > + const int sizeCbCr = (mFrameWidth / 2) * (mFrameHeight / 2); (mFrameWidth+1)/2 * ((mFrameHeight+1)/2 so proper allocation is done should the with or height be odd ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:119 (Diff revision 3) > buffer.mPlanes[1].mWidth = mFrameWidth / 2; > buffer.mPlanes[1].mOffset = 0; > buffer.mPlanes[1].mSkip = 0; > > // Cr plane. > - buffer.mPlanes[2].mData = frame.get(); > + buffer.mPlanes[2].mData = frame.get() + sizeY + sizeCbCr; you could reduce the amount of memory used by 1/3 by making the 3rd plane point to the 2nd plane. I understand that it hurts your eyes. But that's allocating significantly more memory just to get white I must say. in which case you don't need the 3rd memset

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Hi JW, According to :jya's comments, this patch is also modified, please have another look, please.

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; https://reviewboard.mozilla.org/r/65544/#review63856 ::: dom/media/platforms/PDMFactory.cpp:105 (Diff revision 2) > } > } > } > > already_AddRefed<MediaDataDecoder> > PDMFactory::CreateDecoder(const CreateDecoderParams& aParams) I would instead use a CreateBlankDecoder method. Seems easier to me than messing with the CreateDecoderParam. But I don't mind either way.

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; https://reviewboard.mozilla.org/r/64266/#review63860 r+ with my comment below addressed. Thanks! ::: dom/webidl/HTMLMediaElement.webidl:224 (Diff revision 3) > + * This is an API for simulating visibility changes to help debug and write > + * tests about suspend-video-decoding. > + */ > +partial interface HTMLMediaElement { > + [Pref="media.test.setVisible"] > + void mozSetVisible(boolean aVisible); You shouldn't use moz prefixes any more. This is an unfortunate community culture baggage that we need to get rid of. See <https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Guiding_Principles>. :-) Please rename this to setVisible. The fact that this is behind a pref disabled by default is enough.

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; https://reviewboard.mozilla.org/r/65546/#review63858 ::: dom/media/MediaFormatReader.h:436 (Diff revision 3) > RefPtr<SharedTrackInfo> mInfo; > Maybe<media::TimeUnit> mFirstDemuxedSampleTime; > + > + // Use BlankDecoderModule or not. > + bool mIsBlankDecode; > + MozPromiseHolder<GenericPromise> mSwitchDecoderPromise; Why do you need a separate promise to handle switching decoder? Do we even care to know when the decoder has been switched over? After all, this is called when the video element has been hidden, what is going to be returned to the MDSM by that time is irrelevant. It could be a real decoded frame or a blank one it doesn't matter. The MDSM can already start to ignore all returned frames from the time SetBlankDecoder is called, without needing to wait until the change is actually active. ::: dom/media/MediaFormatReader.cpp:725 (Diff revision 3) > LOG("MediaFormatReader called DrainComplete() before flushing, ignoring."); > return; > } > decoder.mDrainComplete = true; > + > + if (!decoder.mSwitchDecoderPromise.IsEmpty()) { I don't believe we need to care about the current decoder having been drained or not. At the time the MDSM calls SetVideoBlandDecode, the element is already hidden. we can already totally ignore the returned framed. There is no need to worry about the decoder being drained nor worry about the ability of the blank decoder to resume as it doesn't decode anything and can take whatever is being thrown at him. The blank decoder will likely require some changes to make sure the timestamps are correct and in the proper order. ::: dom/media/MediaFormatReader.cpp:2061 (Diff revision 3) > + return GenericPromise::CreateAndResolve(true, __func__); > + } > + > + RefPtr<GenericPromise> p = decoder.mSwitchDecoderPromise.Ensure(__func__); > + decoder.mIsBlankDecode = aIsBlankDecode; > + decoder.mNeedDraining = true; I don't think this helps in anyway. There are big impact when you need to drain the decoder. It will automatically cause the video to perform an internal seek to resume to the point where it stopped, greatly confusing the demuxer too. So you don't want to drain, and you don't want to wait for draining until you can delete the current decoder. So long as the next frame returned by the blank decoder is located after the last returned frame. You'll want to reset the Input/Output counter however so that decoding can proceed as is.

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; https://reviewboard.mozilla.org/r/64266/#review63872 ::: modules/libpref/init/all.js:5467 (Diff revision 3) > pref("media.seekToNextFrame.enabled", false); > #else > pref("media.seekToNextFrame.enabled", true); > #endif > > +// Turn on/off the API for simulating visibility changes. We don't need this entry. We only need to change the pref value to true during mochitests.

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; https://reviewboard.mozilla.org/r/65548/#review63878 ::: dom/media/MediaDecoderStateMachine.cpp:1380 (Diff revision 3) > > - if (mIsReaderSuspended) { > + // We should invoke the seek operation after the SetVideoBlankDecode() is > + // done, otherwise the seek operation resets the reader, which also cancels > + // the switching-decoder operation. > + RefPtr<MediaDecoderStateMachine> self = this; > + mSwitchDecoderRequest.Begin(mReader->SetVideoBlankDecode(false) Can you explain how SetVideoBlankDecode() works and why do we wait for the promise before beginning InitiateDecodeRecoverySeek()?

Assigned to Kaku since he has so much fun here!

https://reviewboard.mozilla.org/r/65546/#review63858 > Why do you need a separate promise to handle switching decoder? > > Do we even care to know when the decoder has been switched over? > > After all, this is called when the video element has been hidden, what is going to be returned to the MDSM by that time is irrelevant. > It could be a real decoded frame or a blank one it doesn't matter. > > The MDSM can already start to ignore all returned frames from the time SetBlankDecoder is called, without needing to wait until the change is actually active. I use a promise to make sure that MDSM doesn't invoke a seek operation before the decoder has been switched since the seek operation resets the mNeedDraing flag. But now we are not going to drain the decoder before switching, we don't need this promise.

https://reviewboard.mozilla.org/r/65546/#review63858 > I don't think this helps in anyway. > > There are big impact when you need to drain the decoder. It will automatically cause the video to perform an internal seek to resume to the point where it stopped, greatly confusing the demuxer too. > > So you don't want to drain, and you don't want to wait for draining until you can delete the current decoder. > So long as the next frame returned by the blank decoder is located after the last returned frame. > > You'll want to reset the Input/Output counter however so that decoding can proceed as is. I understand that we don't need to drain the decoder now. But what's your idea to make the blank decoder catch up the original decoder? I come up with the idea that utilizes the decoder.mLastSampleTime (or decoder.mOutput.LastElement()->mTime) to modiify the first output of blank decoder. Does this match your idea?

https://reviewboard.mozilla.org/r/65546/#review63858 > I use a promise to make sure that MDSM doesn't invoke a seek operation before the decoder has been switched since the seek operation resets the mNeedDraing flag. But now we are not going to drain the decoder before switching, we don't need this promise. I would prefer a design where the reader should do seek internally when switching decoders without bothering MDSM. It is really a bad smell in the design whenever we add new features to the reader, MDSM has to change accordingly.

https://reviewboard.mozilla.org/r/65546/#review63858 > I understand that we don't need to drain the decoder now. But what's your idea to make the blank decoder catch up the original decoder? I come up with the idea that utilizes the decoder.mLastSampleTime (or decoder.mOutput.LastElement()->mTime) to modiify the first output of blank decoder. Does this match your idea? yes, I think that should work... I do believe you'll need to modify the blank decoder to output frames in the proper order. Right now it returns frames in decode order which is wrong.

https://reviewboard.mozilla.org/r/64268/#review63842 > you could reduce the amount of memory used by 1/3 by making the 3rd plane point to the 2nd plane. > > I understand that it hurts your eyes. But that's allocating significantly more memory just to get white I must say. > > in which case you don't need the 3rd memset This is quite good, thanks!

Review commit: https://reviewboard.mozilla.org/r/67764/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67764/

Pass the time information of the original decoder's latest decoded sample into the newly created blank decoder so that the blank decoder could adjust its first sample's time. Review commit: https://reviewboard.mozilla.org/r/67766/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67766/

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64266/diff/3-4/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64268/diff/3-4/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/2-3/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/3-4/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/3-4/

https://reviewboard.mozilla.org/r/65548/#review63878 > Can you explain how SetVideoBlankDecode() works and why do we wait for the promise before beginning InitiateDecodeRecoverySeek()? I dropped the promise-based API so this is a non-issue now.

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; https://reviewboard.mozilla.org/r/67764/#review64948 What you could do as a temporary, less optimised solution is if the data is H264 then you set mMaxRefFrames to 16. By spec this is the highest value it can ever be. It will cause unecessary latency in the decoder, but it's still much better than the WMF decoder (That can buffer over 30 frames!) And you do the proper way in a follow up bug ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:35 (Diff revision 1) > > BlankMediaDataDecoder(BlankMediaDataCreator* aCreator, > const CreateDecoderParams& aParams) > : mCreator(aCreator) > , mCallback(aParams.mCallback) > + , mMaxRefFrames(aParams.mConfig.GetType() == TrackInfo::kVideoTrack For this to work, you must guarantee that the video config will contain an extra data (SPS/PSS NAL for h264) If the video is AVC3, the extradata is inband and by default the videoconfig doesn't have one. It also won't work with the other supported video codecs (vp9 and vp8 and soon theora). To guarantee that the videoconfig contains the proper data, you can use the H264Converter ; if videoconfig doesn't contain SPS and PPS NAL, it will postponed the creation of the video decoder until one is seen inband. Only then will it create the decoder with the information it needs. You'll need to amend the BlankDecoderModule so it can be wrapped by the H264Converter and only do so if the data it will receive will be H264. ::: dom/media/platforms/moz.build:61 (Diff revision 1) > ] > > if CONFIG['MOZ_APPLEMEDIA']: > EXPORTS += [ > 'apple/AppleDecoderModule.h', > + 'apple/ReorderQueue.h', you must move that header outside the apple folder; otherwise this will not compile on platforms other than apple.

Comment on attachment 8775598 [details] Bug 1274626 part 4 - make the blank decoders adjust the start time of the first processed sample; https://reviewboard.mozilla.org/r/67766/#review64952 I don't understand why this would be required. If you pass the MediaRawData to the BlankDecoderModule it should use that time. The reordering queue will ensure that the right frame comes out. If you have gap in the returned timestamps because of the frames that got discarded when you flushed/shutdown the active decoders, it won't matter. Fairly sure you can drop that change. If not, please comment it extensively in the commit comment.

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; https://reviewboard.mozilla.org/r/65546/#review64954 ::: dom/media/MediaFormatReader.cpp:2053 (Diff revision 4) > + if (decoder.mIsBlankDecode == aIsBlankDecode) { > + return; > + } > + > + decoder.mIsBlankDecode = aIsBlankDecode; > + decoder.ShutdownDecoder(); shouldn't you want to flush the decoder first. this will also set the counters to 0 and reset any pending draining if there was one. (you'll need to call NotifyDecodingRequested at the end)

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; https://reviewboard.mozilla.org/r/65548/#review64956

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/1-2/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/3-4/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/4-5/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/4-5/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/5-6/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/5-6/

https://reviewboard.mozilla.org/r/67764/#review64994 ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:24 (Diff revision 2) > #include "TimeUnits.h" > #include "VideoUtils.h" > > namespace mozilla { > > +extern uint32_t ComputeMaxRefFrames(const MediaByteBuffer* aExtraData); The definition of ComputeMaxRefFrames() should also be moved out of the apple folder, otherwise it won't compile on other platforms.

https://reviewboard.mozilla.org/r/67764/#review64994 > The definition of ComputeMaxRefFrames() should also be moved out of the apple folder, otherwise it won't compile on other platforms. Not sure where to put the definition. I will move the definition to the BlankDecoderModule.cpp and make the one in the AppleVTDecoder.cpp an extern deceleration

https://reviewboard.mozilla.org/r/65544/#review65058 ::: dom/media/platforms/PDMFactory.cpp:283 (Diff revision 4) > > +void > +PDMFactory::CreateBlankPDM() > +{ > + mBlankPDM = CreateBlankDecoderModule(); > + StartupPDM(mBlankPDM); Should not call StartupPDM() here, which puts the blank decoder into mCurrentPDMs. This cause some mochitest failures.

https://reviewboard.mozilla.org/r/67764/#review64994 > Not sure where to put the definition. I will move the definition to the BlankDecoderModule.cpp and make the one in the AppleVTDecoder.cpp an extern deceleration why don't you put it in the SPS class? Or within the H264Wrapper ; it's more logical to me that it's there than in the BlankDecoder

https://reviewboard.mozilla.org/r/67764/#review64994 > why don't you put it in the SPS class? > Or within the H264Wrapper ; it's more logical to me that it's there than in the BlankDecoder Sure, will do. I am not familiar with the structure here, thanks for your suggestion!

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/2-3/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/4-5/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/6-7/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/6-7/

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; https://reviewboard.mozilla.org/r/67764/#review65288 ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:40 (Diff revision 3) > + , mMaxRefFrames(0) > , mType(aParams.mConfig.GetType()) > { > + if (aParams.mConfig.GetType() == TrackInfo::kVideoTrack && > + H264Converter::IsH264(aParams.mConfig)) { > + mp4_demuxer::H264::ComputeMaxRefFrames(aParams.VideoConfig().mExtraData); and what do you do with the returned value ?? ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:111 (Diff revision 3) > + } > + > +private: > nsAutoPtr<BlankMediaDataCreator> mCreator; > MediaDataDecoderCallback* mCallback; > + const uint32_t mMaxRefFrames; the way it's initialised, you can't make that const (once you fix the initialisation problem) ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:274 (Diff revision 3) > } > > ConversionRequired > DecoderNeedsConversion(const TrackInfo& aConfig) const override > { > + if (aConfig.IsVideo()) { for clarity,it would be better to only return kNeedAVCC if the configuration is for h264

https://reviewboard.mozilla.org/r/67764/#review65288 > and what do you do with the returned value ?? I feel shame for this... Will assign the result to mMaxRefFrames and make the mMaxRefFrames non-const.

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64266/diff/4-5/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64268/diff/4-5/

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/3-4/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/5-6/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/7-8/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/7-8/

you could keep it const and in the constructor do something like mMaxRefFrames(aParams.mConfig.GetType() == TrackInfo::kVideoTrack && H264Converter::IsH264(aParams.mConfig) ? : mp4_demuxer::H264::ComputeMaxRefFrames(aParams.VideoConfig().mExtraData) ? 0 Which is what JW changed form my earlier code doing like you suggest

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/4-5/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/6-7/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/8-9/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/8-9/

https://reviewboard.mozilla.org/r/67764/#review65322 ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:58 (Diff revision 5) > RefPtr<MediaData> data = > mCreator->Create(media::TimeUnit::FromMicroseconds(aSample->mTime), > media::TimeUnit::FromMicroseconds(aSample->mDuration), > aSample->mOffset); > - if (!data) { > - mCallback->Error(MediaDataDecoderError::FATAL_ERROR); > + > + OutputFrame(data.get()); the get() is unecessaru, as RefPtr<T> has a () operator that will perform the conversion to T* automatically ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:73 (Diff revision 5) > } > > - nsresult Drain() override { > + nsresult Drain() override > + { > + while (!mReorderQueue.IsEmpty()) { > + mCallback->Output(mReorderQueue.Pop().get()); don't need the get here either.

https://reviewboard.mozilla.org/r/67764/#review65322 > the get() is unecessaru, as RefPtr<T> has a () operator that will perform the conversion to T* automatically Oh, yup. I do it on purpose to get rid of my editor's warnning. My editor is so stupid that it does not know the conversion. Will remove it.

https://reviewboard.mozilla.org/r/67764/#review65322 > don't need the get here either. Do not apply to this one since converting a r-value RefPtr<T> to T* is forbidden here http://searchfox.org/mozilla-central/source/mfbt/RefPtr.h#294 .

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/5-6/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/7-8/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/9-10/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/9-10/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9731f698f61d Looks good.

Thanks for the reviews!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7f310f128d Part 1 - add a method to HTMLMediaElement for debugging visibilty change; r=jwwang,ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/a722438c2be6 Part 2 - make the blank video data creator return a white image; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/12e3777de139 Part 3 - make the blank video decoder return samples in PTS order; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/222b4883bf79 Part 4 - provide APIs to create blank decoders; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b79d4a6913 Part 5 - provide APIs to switch to blank decoders dynamically; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/221e8d2b180e Part 6 - make MDSM change video decoder dynamically; r=jwwang

https://hg.mozilla.org/mozilla-central/rev/eb7f310f128d https://hg.mozilla.org/mozilla-central/rev/a722438c2be6 https://hg.mozilla.org/mozilla-central/rev/12e3777de139 https://hg.mozilla.org/mozilla-central/rev/222b4883bf79 https://hg.mozilla.org/mozilla-central/rev/c5b79d4a6913 https://hg.mozilla.org/mozilla-central/rev/221e8d2b180e

had to back this out since this broke unified builds after a clobber like https://treeherder.mozilla.org/logviewer.html#?job_id=4539803&repo=mozilla-central

Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/a33effc45c9e Backed out changeset 221e8d2b180e https://hg.mozilla.org/mozilla-central/rev/746d944389b2 Backed out changeset c5b79d4a6913 https://hg.mozilla.org/mozilla-central/rev/14c8480b5e58 Backed out changeset 222b4883bf79 https://hg.mozilla.org/mozilla-central/rev/1f0d8ecf400a Backed out changeset 12e3777de139 https://hg.mozilla.org/mozilla-central/rev/4a3d68cc57ba Backed out changeset a722438c2be6 https://hg.mozilla.org/mozilla-central/rev/6608e5864780 Backed out changeset eb7f310f128d for breaking unified builds

Here, https://hg.mozilla.org/mozilla-central/rev/100d3954e65d, the IsH264() static utility is moved form H264Converter to MP4Decoder. Will update the patches soon.

Comment on attachment 8770989 [details] Bug 1274626 part 1 - add a method to HTMLMediaElement for debugging visibilty change; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64266/diff/5-6/

Comment on attachment 8770990 [details] Bug 1274626 part 2 - make the blank video data creator return a white image; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64268/diff/5-6/

Comment on attachment 8775597 [details] Bug 1274626 part 3 - make the blank video decoder return samples in PTS order; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67764/diff/6-7/

Comment on attachment 8772807 [details] Bug 1274626 part 4 - provide APIs to create blank decoders; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65544/diff/8-9/

Comment on attachment 8772808 [details] Bug 1274626 part 5 - provide APIs to switch to blank decoders dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65546/diff/10-11/

Comment on attachment 8772809 [details] Bug 1274626 part 6 - make MDSM change video decoder dynamically; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65548/diff/10-11/

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3afceea687 part 1 - add a method to HTMLMediaElement for debugging visibilty change; r=jwwang,ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/876f15c4bfff part 2 - make the blank video data creator return a white image; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b41bc89af1 part 3 - make the blank video decoder return samples in PTS order; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/fd97d47edbb9 part 4 - provide APIs to create blank decoders; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/e15f10941eed part 5 - provide APIs to switch to blank decoders dynamically; r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/4adc486250b6 part 6 - make MDSM change video decoder dynamically; r=jwwang

https://hg.mozilla.org/mozilla-central/rev/ee3afceea687 https://hg.mozilla.org/mozilla-central/rev/876f15c4bfff https://hg.mozilla.org/mozilla-central/rev/a2b41bc89af1 https://hg.mozilla.org/mozilla-central/rev/fd97d47edbb9 https://hg.mozilla.org/mozilla-central/rev/e15f10941eed https://hg.mozilla.org/mozilla-central/rev/4adc486250b6

Too late to fix in 50.1.0 release

I reproduced this issue using Fx 49.0a1, build ID: 20160520030251, on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 51.0b10, build ID: 20161222080852, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11. Cheers!