Some reported playback issues seem to be related to slow hardware (either slow software decoding, or even too-slow hardware decoding of high-resolution videos). To be able to better reproduce these situations during development and testing, it would be useful to have a way to introduce a delay during decoding. To hep with reproducibility, this delay should be such that the total decoding+delay time would be the same on any machine (provided the actual decoding time is not longer to start with, of course). This simulated decoding time value should probably be set through a hidden about:config preference, so that QA may use it in automated tests.

bholley mentioned related ideas for media "chaos mode" testing in bug 1134837.

I've already got this feature built into my 2008 Dell Laptop.

Is this a duplicate of bug 1115759?

(In reply to Chris Peterson [:cpeterson] from comment #1) > bholley mentioned related ideas for media "chaos mode" testing in bug > 1134837. Thanks. (In reply to Bobby Holley (:bholley) from comment #3) > Is this a duplicate of bug 1115759? Thanks for the pointer. Same goal, different means I think, so we might want to keep them separate in case we want to implement your proposal. I've got a WIP patch that intercepts video frames in MediaFormatReader::NotifyNewOutput and ensures they're actually injected X ms apart from each other. Will post in the next few days.

WIP patch, feedback welcome. If a decoded video frame arrives sooner than 30ms after the previous one, it is queued separately and later properly re-queued (as if it had just been decoded). The 30ms value can easily be changed in the code; later it could be read from about:config. This code currently hangs on FF shutdown! Not sure yet if it's an issue with this patch (very likely) or if it's highlighting a real problem in the existing shutdown code.

Comment on attachment 8649076 [details] [diff] [review] 1194518-minimum-frame-output-interval-WIP.patch Review of attachment 8649076 [details] [diff] [review]: ----------------------------------------------------------------- looks like I didn't validate yesterday.. sorry for the delay. --- Will make a mess in MediaFormatReader. I wonder if we could stick that in a wrapper like H264Converter or similar to SharedDecoderManager which had a proxy callback ; so this stuff is totally separate and can be easily added at will, including via a pref. ::: dom/media/MediaFormatReader.cpp @@ +826,5 @@ > + > +bool MediaFormatReader::DoOutputFrame(TrackType aTrack, MediaData* aSample) > +{ > + MOZ_ASSERT(OnTaskQueue()); > + mLastVideoOutputReceived = TimeStamp::Now(); you audio samples will pollute your timestamp here. need to only update this if aTrack == TrackInfo::kVideoTrack. You typically have much more audio samples that video coming out of the decoders ; this would increase the delay by far more than 30ms ::: dom/media/MediaFormatReader.h @@ +442,5 @@ > #endif > + > + static const TimeDuration mMinimumVideoFrameOutputInterval; > + TimeStamp mLastVideoOutputReceived; > + nsTArray<nsRefPtr<MediaData>> mDelayedOutput; wouldn't a dequeue be more appropriate?

Fully reworked the patch to be a wrapper, as suggested in comment 6. Part 1: Simple wrapper that just passes all calls and callbacks straight through. It will make reviewing the actual fuzzing code easier (see part 3). Also it is useful in itself, to easily spy on the traffic to and from the wrapped decoder.

Part 2: Engage the wrapper around the video decoder if the pref 'media.fuzz.video-decode-passthrough' is true. If the env-var 'MediaFuzzingWrapper' is set to 5, the wrapper will log all calls to and from the wrapped decoder.

Part 3: Fuzzing the decoder output, by delaying frames (if necessary) so that they are always separated by at least a certain time duration. The goal is to be able to reproduce a certain output rate on any test hardware, to hopefully get consistent results when testing or debugging. Flushing/draining/shutting-down just clears the delayed queue. No delaying of InputExhausted, we could implement that later if deemed necessary. Note: This implementation uses an nsTArray to store delayed frames; another patch will switch to std::queue.

Part 4: Actually use the delaying wrapper when the pref 'media.fuzz.video-decode-minimum-frame-interval-ms' is non-zero.

Comment on attachment 8651607 [details] [diff] [review] 1194518-p1-fuzzing-wrapper-passthrough.patch Review of attachment 8651607 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp @@ +24,5 @@ > + , mCallback(aCallback) > +{ > + FW_LOGV("aTaskQueue=%p aCallback=%p", aTaskQueue, aCallback); > + MOZ_ASSERT(mTaskQueue); > + MOZ_ASSERT(mCallback); this will crash on windows as to test if a decoder can be properly created (e.g. if the platform isn't broken) it creates a decoder with a null taskqueue and null callback. ::: dom/media/platforms/wrappers/FuzzingWrapper.h @@ +10,5 @@ > +#include "PlatformDecoderModule.h" > + > +namespace mozilla { > + > +class FuzzingWrapper : public MediaDataDecoder, public MediaDataDecoderCallback I personally dislike multiple-inheritance and I would much prefer a separate class that does just the MediaDataDecoderCallBack. If you need one to access the member of the other, it can be made friend

Part 5: Using std::queue instead of nsTArray. Not sure if it's really necessary*, hence the separate patch. Ideally we should profile before choosing between the two! * There shouldn't be too many frames being delayed at once; This patch is about *delaying* things, so wasting some time is not important; std::queue is more opaque to debug than nsTArray.

Comment on attachment 8651608 [details] [diff] [review] 1194518-p2-use-passthrough-fuzzing-wrapper.patch Review of attachment 8651608 [details] [diff] [review]: ----------------------------------------------------------------- The fuzzing should be done inside the PlatformDecoderModule::CreateDecoder. The MediaFormatReader should have no concept of wrapper. the existing use of the SharedDecoderManager is an aberration and will be removed,

Comment on attachment 8651612 [details] [diff] [review] 1194518-p3-fuzzing-wrapper-min-output-interval.patch Review of attachment 8651612 [details] [diff] [review]: ----------------------------------------------------------------- I believe drain implementation is wrong as it will drop delayed frames. Please have clear moz_assert about which taskqueue is used in each function member. Starting to become difficult to follow when you have multiple taskqueues at play. ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp @@ +37,5 @@ > MOZ_ASSERT(mDecoder); > } > > +void > +FuzzingWrapper::SetMinimumFrameOutputInterval(TimeDuration aMinimumFrameOutputInterval) where is this called from? where is the documentation? @@ +160,5 @@ > + MonitorAutoLock mon(mMonitor); > + if (mDelayedOutput.IsEmpty()) { > + return; > + } > + MediaData* data = mDelayedOutput[0]; nsRefPtr<MediaData> for clarity @@ +198,5 @@ > void > FuzzingWrapper::DrainComplete() > { > FW_LOGV(""); > + ClearDelayedOutput(); you're not draining your queued delayed frames. What will happen to those? Shouldn't you wait to ensure those have been properly returned to the reader first?

Comment on attachment 8651613 [details] [diff] [review] 1194518-p4-use-delaying-fuzzing-wrapper.patch Review of attachment 8651613 [details] [diff] [review]: ----------------------------------------------------------------- Like P2. this should be handled within the PDM

Comment on attachment 8651614 [details] [diff] [review] 1194518-p5-stdqueue.patch Review of attachment 8651614 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp @@ +162,3 @@ > return; > } > + MediaData* data = mDelayedOutput.front(); nsRefPtr<MediaData>

There are a few things I disagree with, so I'd prefer to discuss them first before I spend too much time on something that I had hoped would be a *quick* attempt to fuzz some things, and would only be used by Mozilla developers and testers. --- (In reply to Jean-Yves Avenard [:jya] from comment #11) > Comment on attachment 8651607 [details] [diff] [review] > 1194518-p1-fuzzing-wrapper-passthrough.patch > > Review of attachment 8651607 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp > @@ +27,2 @@ > > + MOZ_ASSERT(mTaskQueue); > > + MOZ_ASSERT(mCallback); > > this will crash on windows as to test if a decoder can be properly created > (e.g. if the platform isn't broken) it creates a decoder with a null > taskqueue and null callback. Agreed, easy to fix. Will do. > ::: dom/media/platforms/wrappers/FuzzingWrapper.h > @@ +14,1 @@ > > +class FuzzingWrapper : public MediaDataDecoder, public MediaDataDecoderCallback > > I personally dislike multiple-inheritance and I would much prefer a separate > class that does just the MediaDataDecoderCallBack. If you need one to access > the member of the other, it can be made friend I've started looking into this, but it makes things much more complicated than I think is necessary for this simple class. Will NOT do. (In reply to Jean-Yves Avenard [:jya] from comment #13) > Comment on attachment 8651608 [details] [diff] [review] > 1194518-p2-use-passthrough-fuzzing-wrapper.patch > > Review of attachment 8651608 [details] [diff] [review]: > ----------------------------------------------------------------- > > The fuzzing should be done inside the PlatformDecoderModule::CreateDecoder. > > The MediaFormatReader should have no concept of wrapper. Makes sense. Will do. (In reply to Jean-Yves Avenard [:jya] from comment #14) > Comment on attachment 8651612 [details] [diff] [review] > 1194518-p3-fuzzing-wrapper-min-output-interval.patch > > Review of attachment 8651612 [details] [diff] [review]: > ----------------------------------------------------------------- > > I believe drain implementation is wrong as it will drop delayed frames. [ also: ] > @@ +198,5 @@ > > void > > FuzzingWrapper::DrainComplete() > > { > > FW_LOGV(""); > > + ClearDelayedOutput(); > > you're not draining your queued delayed frames. What will happen to those? > > Shouldn't you wait to ensure those have been properly returned to the reader > first? The documentation of MediaDataDecoder::drain() says "If the decoder can't produce samples from the current output, it drops the input samples", so it should be fine to drop delayed frames, as we're simulating that the decoder is slow in outputting frames, so they could very well still be in the non-existent decoder pipeline with no chance of being decoded at the time of the drain(). Will NOT do. (However if in the future there is value in simulating the output of some/all of these delayed frames, this could be added, through another bug.) > Please have clear moz_assert about which taskqueue is used in each function > member. Starting to become difficult to follow when you have multiple > taskqueues at play. I was hoping to avoid having to deal with task queues, as this wrapper is almost like a pass-through: Calls to MediaDataDecoder and MediaDataDecoderCallback methods happen on some media-related queues and are usually forwarded from the same queues. This wrapper just holds some frames for delayed output, and we know that the final callback (in MediaFormatReader) just does re-posts the message to its preferred queue, so it doesn't matter where it comes from. The main concurrency comes from adding/removing frames to/from the frame queue, which I deal with through a mutex for simplicity. If you really really think I should use particular task queues to avoid using a mutex, then fine I can do it, but it may be significant work. > ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp > @@ +41,1 @@ > > +FuzzingWrapper::SetMinimumFrameOutputInterval(TimeDuration aMinimumFrameOutputInterval) > > where is this called from? where is the documentation? It's called from the following patch. :-) The name of the method seems self-explanatory to me, but if you'd really like me to write redundant documentation, then fine I can do it. > @@ +164,1 @@ > > + MediaData* data = mDelayedOutput[0]; > > nsRefPtr<MediaData> for clarity [ also: ] (In reply to Jean-Yves Avenard [:jya] from comment #16) > Comment on attachment 8651614 [details] [diff] [review] > 1194518-p5-stdqueue.patch > > Review of attachment 8651614 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp > @@ +164,1 @@ > > + MediaData* data = mDelayedOutput.front(); > > nsRefPtr<MediaData> I don't think an nsRefPtr is needed here, as mDelayedOutput keeps an nsRefPtr itself (until the 'RemoveElement' below), and we pass data to mCallback->Output(), which takes a MediaData*. In face, the only reason I have this temporary 'data' is because I also use data in a logging statement; otherwise I would just have done mCallback->Output(mDelayedOutput[0]); What kind of clarity do you think your nsRefPtr would bring? (at the cost of an extra pair of Add/Release) But if you really want it, fine I'll do it. (In reply to Jean-Yves Avenard [:jya] from comment #15) > Comment on attachment 8651613 [details] [diff] [review] > 1194518-p4-use-delaying-fuzzing-wrapper.patch > > Review of attachment 8651613 [details] [diff] [review]: > ----------------------------------------------------------------- > > Like P2. this should be handled within the PDM Ok will do. --- There are two "Will NOT do" above, do you insist on them?

(In reply to Gerald Squelart [:gerald] from comment #17) > I've started looking into this, but it makes things much more complicated > than I think is necessary for this simple class. > Will NOT do. > ok then. > The documentation of MediaDataDecoder::drain() says "If the decoder can't > produce samples from the current output, it drops the input samples", so it > should be fine to drop delayed frames, as we're simulating that the decoder > is slow in outputting frames, so they could very well still be in the > non-existent decoder pipeline with no chance of being decoded at the time of > the drain(). this is the documentation extracted from the WMF framework and you're reading it wrong. The only reason the decoder wouldn't produce produce samples from the input is because a frame dependency is not satisfied. Here the decoder has received the input it's just "too slow" to output it right away. It will be decoded upon draining. The only frames that are dropped on draining are the ones that can't ever be decoded from the current stream received so far. Dropping frames that could have been decoded will only mean that you're going to fail some tests. Draining is typically called upon reaching the end of the file. It's to tell the decoder that no more input will be happening and it's okay to DRAIN its current buffer, and stop waiting for new ones. The aim of this bug is to simulate slow hardware, not emulate a broken one. If you drop frames that would otherwise have been decoded, and this is an intermittent behaviour, then you're going to introduce intermittent errors in the tests: like "ended" won't be reached or currentTime won't be at the expected value. Chasing those intermittent will distract you from the ultimate goal of finding improper A/V sync behaviour > Will NOT do. you should :) > > (However if in the future there is value in simulating the output of > some/all of these delayed frames, this could be added, through another bug.) > > > Please have clear moz_assert about which taskqueue is used in each function > > member. Starting to become difficult to follow when you have multiple > > taskqueues at play. > > I was hoping to avoid having to deal with task queues, as this wrapper is > almost like a pass-through: Calls to MediaDataDecoder and > MediaDataDecoderCallback methods happen on some media-related queues and are > usually forwarded from the same queues. no dealing with taskqueue (so you won't have to deal with monitors) is a different topic than getting a good understanding on what run where. Asserting that a particular code runs on a particular task queue only clarify the picture and leaves no ambiguity. It will also ensure that you can't ever have deadlocks (as we would have asserted long ago) > This wrapper just holds some frames for delayed output, and we know that the > final callback (in MediaFormatReader) just does re-posts the message to its > preferred queue, so it doesn't matter where it comes from. > The main concurrency comes from adding/removing frames to/from the frame > queue, which I deal with through a mutex for simplicity. > > If you really really think I should use particular task queues to avoid > using a mutex, then fine I can do it, but it may be significant work. I'm no fuss on using taskqueue ; but I'm fuss on asserting that the code runs where it's expected to run. If you check the last round of cleanup done by Bobby and myself, it's all been to ensure we have a very clear threading model. > > > + MediaData* data = mDelayedOutput.front(); > > > > nsRefPtr<MediaData> > > I don't think an nsRefPtr is needed here, as mDelayedOutput keeps an > nsRefPtr itself (until the 'RemoveElement' below), and we pass data to > mCallback->Output(), which takes a MediaData*. yes, it's totally unnecessary *now*. But this isn't what this is about: it's a coding-style issue. You ensure that you'll never have dangling pointer not now, nor in the future where someone decide to return early or whatever. (and I got that exact same comment too in my first patches, and I had made the same comment as you) > In face, the only reason I have this temporary 'data' is because I also use > data in a logging statement; otherwise I would just have done > mCallback->Output(mDelayedOutput[0]); yes, I had noticed that. If you want to avoid the unnecessary the AddRef/Release change that to: nsRefPtr<MediaData>& data = mDelayedOutput.front() / mDelayedOutput[0]; Then there's no ambiguity whatsoever and you avoid the implicit conversion from nsRefPtr<T> to T* as well the the refcount change. > > There are two "Will NOT do" above, do you insist on them? just the drain for the above reasons.

Update of p1 as per comment 11: Removed wrong asserts, split class in two to avoid multiple inheritance. Carrying r+.

Update of p2 as per comment 13: Fuzzing wrapper created in PlatformDecoderModule.

Update of p3 as per comment 14: Drain now outputs all delayed frames; added documentation; no more naked MediaData pointer.

Update of p4 as per comment 15: Fuzzing wrapper created and setup in PDM.

Rebase of p5, but with a deque instead of queue, as I need to access both ends. Carrying r+.

An 'explicit' is missing for the single-argument constructor in the first patch, please assume I'll add it after your reviews.

Comment on attachment 8652719 [details] [diff] [review] 1194518-p2-use-passthrough-fuzzing-wrapper.patch v2 Review of attachment 8652719 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/PlatformDecoderModule.cpp @@ +248,5 @@ > + > + if (callbackWrapper && m) { > + nsRefPtr<DecoderFuzzingWrapper> wrapper = > + new DecoderFuzzingWrapper(m.forget(), callbackWrapper.forget()); > + m = wrapper.forget(); why bother with the wrapper local variable ? m = new blah

Comment on attachment 8652721 [details] [diff] [review] 1194518-p3-fuzzing-wrapper-min-output-interval.patch v2 Review of attachment 8652721 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp @@ +81,5 @@ > + // Shutdown may block a bit. > + nsresult result = mDecoder->Shutdown(); > + DFW_LOGV("Shutting down mTaskQueue"); > + mCallbackWrapper->mTaskQueue->BeginShutdown(); > + mCallbackWrapper->mTaskQueue->AwaitIdle(); it would be nicer to have a public method in the callback wrapper instead ::: dom/media/platforms/wrappers/FuzzingWrapper.h @@ +72,5 @@ > + bool mDelayInputExhaustedToMatchOutput; > + // Members for minimum frame output interval & InputExhausted, > + // should only be accessed on mTaskQueue. > + TimeStamp mPreviousOutput; > + // Bool is true if an 'InputExhausted' arrived after this frame that we're delaying. the frame we're delaying?

Comment on attachment 8652722 [details] [diff] [review] 1194518-p4-use-delaying-fuzzing-wrapper.patch v2 Review of attachment 8652722 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/PlatformDecoderModule.cpp @@ +87,5 @@ > > Preferences::AddBoolVarCache(&sUsePassthroughWrapper, > "media.fuzz.video-decode-passthrough", false); > + Preferences::AddUintVarCache(&sMinimumVideoFrameOutputInterval_ms, > + "media.fuzz.video-decode-minimum-frame-interval-ms", false); you probably want a default value here other than "false" seeing that the pref is an int. Do we even need the media.fuzz.video-decode-passthrough pref seeing that you test if the value isn't 0 *and* that the pref is not set, it now seems redundant

Added missing 'explicit'. Carrying r+ from comment 11.

Small update as per comment 25; Also after discussion with :jya, renamed pref to 'media.decoder.fuzzing.enabled'. Carrying r+ from comment 25.

Small update as per comment 26; Also by default InputExhausted are delayed to match the video output. Carrying r+ from comment 26.

Small update as per comment 27; Renamed prefs to 'media.decoder.fuzzing.video-output-minimum-interval-ms' and 'media.decoder.fuzzing.dont-delay-inputexhausted'. Carrying r+ from comment 27.

Rebase. Carrying r+ from comment 16.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5709b0f4bd9 https://hg.mozilla.org/integration/mozilla-inbound/rev/63aff0c6aa12 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa665c51b21a https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e3497da6f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/45352395d21a

https://hg.mozilla.org/mozilla-central/rev/d5709b0f4bd9 https://hg.mozilla.org/mozilla-central/rev/63aff0c6aa12 https://hg.mozilla.org/mozilla-central/rev/aa665c51b21a https://hg.mozilla.org/mozilla-central/rev/b8e3497da6f7 https://hg.mozilla.org/mozilla-central/rev/45352395d21a