The current decode ahead logic is flawed. Currently all MediaDataDecoder always call InputExhausted after processing an input, regardless of them needing more data. InputExhausted should only be called if the MediaDataDecoder has exhausted its input but hasn't produced an output yet. There are various places in the MediaFormatReader making assumptions based on the amount of Input vs amount of Output. They are mostly wrong. An example would be if a decoder output multiple samples from a single output. Which could occur if for example the mp4 was badly muxed and the mdat box contained more than one sample. This would cause input - output to be negative, as they are unsigned int that would make a very big number. And as such the test > 2 would always be true. We should rework the logic such as there's no mode decode ahead (the benefits were always dubious at best) and prevent the MDSM from knowing exactly where decoding status is at. We should make InputExhausted only be used if the decoder didn't return content because it has exhausted all its input. If the decoder return an input, then inputexhausted shouldn't be called. It would give us a more deterministic outcome, and prevent some random intermittent playback stalls.

Comment on attachment 8786212 [details] Bug 1297265: P3. Rework Apple VT use of InputExhausted. https://reviewboard.mozilla.org/r/75200/#review73054 Carrying r+ from bug 1287379

Comment on attachment 8786312 [details] Bug 1297265: P3. Rework Apple VT use of InputExhausted. https://reviewboard.mozilla.org/r/75288/#review73112

not sure what happened with mozreview here, but it only wanted to push the last commit. so had to have multiple goes until it worked.

Comment on attachment 8786310 [details] Bug 1297265: P1. Remove decode ahead logic. https://reviewboard.mozilla.org/r/75284/#review73790

Comment on attachment 8786311 [details] Bug 1297265: P2. Amend MediaDataDecoder documentation to emphasize the new expected behavior. https://reviewboard.mozilla.org/r/75286/#review73792

Comment on attachment 8786313 [details] Bug 1297265: P4. Rework Apple AudioToolbox use of InputExhausted. https://reviewboard.mozilla.org/r/75290/#review73794

Comment on attachment 8786314 [details] Bug 1297265: P5. Rework Blank Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75292/#review73796

Comment on attachment 8786315 [details] Bug 1297265: P6. Rework Opus Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75294/#review73798

Comment on attachment 8786316 [details] Bug 1297265: P7. Rework Theora Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75296/#review73800

Comment on attachment 8786317 [details] Bug 1297265: P8. Rework LibVPX Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75298/#review73802

Comment on attachment 8786318 [details] Bug 1297265: P9. Rework Vorbis Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75300/#review73804

Comment on attachment 8786319 [details] Bug 1297265: P10. Rework FFmpeg Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75302/#review73806

Comment on attachment 8786220 [details] Bug 1297265: P11. Rework WMF Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/75216/#review73808

Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dd4c7aafcc3d P1. Remove decode ahead logic. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/da9117375d2e P2. Amend MediaDataDecoder documentation to emphasize the new expected behavior. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/ec1f3ac8751c P3. Rework Apple VT use of InputExhausted. r=me https://hg.mozilla.org/integration/autoland/rev/345a577c92a1 P4. Rework Apple AudioToolbox use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/cc70d3a139bc P5. Rework Blank Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/30636ac0790f P6. Rework Opus Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/e6b9e6184457 P7. Rework Theora Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/67a97304d44d P8. Rework LibVPX Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/ad1915ff688b P9. Rework Vorbis Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/c7bffd1ba7fd P10. Rework FFmpeg Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/04d9b7160ac9 P11. Rework WMF Decoder use of InputExhausted. r=kamidphish

Backed out in https://hg.mozilla.org/integration/autoland/rev/6658a683e292 for gtest timeouts in MediaDataDecoder.H264 like https://treeherder.mozilla.org/logviewer.html#?job_id=2884639&repo=autoland

Crashtest failures too. https://treeherder.mozilla.org/logviewer.html#?job_id=2886249&repo=autoland

Try runs were all green :(

(In reply to Ryan VanderMeulen [:RyanVM] from comment #52) > Crashtest failures too. > https://treeherder.mozilla.org/logviewer.html#?job_id=2886249&repo=autoland there's nothing media related down that path :(

Comment on attachment 8787139 [details] Bug 1297265: P12. Rework WAV Decoder use of InputExhausted. https://reviewboard.mozilla.org/r/76002/#review73944

Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4af76d3daf0a P1. Remove decode ahead logic. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/2a5c95235381 P2. Amend MediaDataDecoder documentation to emphasize the new expected behavior. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/9f88eccfceaf P3. Rework Apple VT use of InputExhausted. r=me https://hg.mozilla.org/integration/autoland/rev/ad81e05890aa P4. Rework Apple AudioToolbox use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/110bb999d8d4 P5. Rework Blank Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/4f4690f0bf00 P6. Rework Opus Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/b1e54cffce98 P7. Rework Theora Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/b45017d6e8c1 P8. Rework LibVPX Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/4f53b8e00ae9 P9. Rework Vorbis Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/c2ccd9ef82a2 P10. Rework FFmpeg Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/3809ef6d5f51 P11. Rework WMF Decoder use of InputExhausted. r=kamidphish https://hg.mozilla.org/integration/autoland/rev/7db02dab044e P12. Rework WAV Decoder use of InputExhausted. r=gerald

https://hg.mozilla.org/mozilla-central/rev/4af76d3daf0a https://hg.mozilla.org/mozilla-central/rev/2a5c95235381 https://hg.mozilla.org/mozilla-central/rev/9f88eccfceaf https://hg.mozilla.org/mozilla-central/rev/ad81e05890aa https://hg.mozilla.org/mozilla-central/rev/110bb999d8d4 https://hg.mozilla.org/mozilla-central/rev/4f4690f0bf00 https://hg.mozilla.org/mozilla-central/rev/b1e54cffce98 https://hg.mozilla.org/mozilla-central/rev/b45017d6e8c1 https://hg.mozilla.org/mozilla-central/rev/4f53b8e00ae9 https://hg.mozilla.org/mozilla-central/rev/c2ccd9ef82a2 https://hg.mozilla.org/mozilla-central/rev/3809ef6d5f51 https://hg.mozilla.org/mozilla-central/rev/7db02dab044e

This patch did a performance improvement in talos, thanks. Improvements: 9% basic_compositor_video summary osx-10-10 opt e10s 13.6 -> 12.35 8% basic_compositor_video summary osx-10-10 opt 11.64 -> 10.75 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2939

Please define what performance means when it comes to media playback... All those changes do is to not decode object in advance. In a given amount of time, the amount of processing required would be identical. Also, what is more important: that a video plays fine without stuttering or that some particular component around the video happened to be drawn a few percentage point quicker. Had talos performed the measurement on a different time, results would have been different. to me this is only further proof that talos indication is nonsensical in the context of media playback and it's just measuring the wrong thing. things that are totally irrelevant to the user experience.

:jya, here is a compare view of talos for this patch: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=d29b950695f42eaa02de85e40c18ba65eea29c50&newProject=autoland&newRevision=7db02dab044ea63188daa8a1f04ab29647e3b139&originalSignature=a75c7fb871569a3321ba0c9065fd64b3db159a18&newSignature=a75c7fb871569a3321ba0c9065fd64b3db159a18&framework=1 you can see a lot of improvements on the 1080p tests, but the 240p tests at scale 1 have regressions. I am including :ethlin on here has he wrote the original test and can answer your questions specifically about the test. :jya, please file bugs if you determine a Talos test is not useful and we can determine if it needs to be changed, deleted, or documented better. That would be a productive way to complain about Talos :)

(In reply to Jean-Yves Avenard [:jya] from comment #80) > Please define what performance means when it comes to media playback... > > All those changes do is to not decode object in advance. In a given amount > of time, the amount of processing required would be identical. > Also, what is more important: that a video plays fine without stuttering or > that some particular component around the video happened to be drawn a few > percentage point quicker. > Had talos performed the measurement on a different time, results would have > been different. > > to me this is only further proof that talos indication is nonsensical in the > context of media playback and it's just measuring the wrong thing. things > that are totally irrelevant to the user experience. The original idea of the basic_compositor_video test comes from Bug 1253062 - Improve unaccelerated video performance. There are some memory copy and color space conversion from decoder to compositor and we try to remove them. We want to measure the improvement from the test. So the basic idea of the test is to play a video clip as soon as possible and see how many frames we can get from the compositor. The reason of playing ASAP is because some devices may easily play the clip in 60fps. The output will be ms/frame. I'm not familiar with the media source code, so probably there is some factors which may effect the result and I didn't consider them in the test. For instance, I don't know we decode object in advance. But from the test result, it really effects the ms/frame for certain duration of the video. :jya, do you have any idea how to adjust the basic_compositor_video test to be more relevant to user experience? We can have a bug to improve it.

There's something fundamentally flawed in your approach; and in the whole concept of measuring something that is clock controlled. You can't just time how long the compositor takes to render a particular frame. You are measuring how long it takes to demux a sample, decode the frame, wait until it's time to display that sample, and then the sample to be rendered. If you had say demuxed and decoded all videos at once, and only then measured how long it took to render those frames: then yes, you would have been able to measure the compositor. There's no point measuring how many frames you get to the compositor, because that number is clock dependent. What you are in effect measuring is how accurate the hardware (or software) system clock is. We have utility classes to measure how long decoding takes. It could also help writing some code to decode those in block and only then attempt to render them as fast as possible. You could also For your test to be valid, you must remove the system clock from the equation. It is possible following this change that decoding would have started slightly quicker which would have affected your measurements, but those are irrelevant because the video would have finished playing in the same amount of time (being clock controlled)

It made bug 1296211 go worse on mac. We can see in the case where it works that we had only 1 or 2 VT decode error, but somehow now we almost consistently get 4 errors which causes the element to go into error and the test will fail.

(In reply to Jean-Yves Avenard [:jya] from comment #80) > Please define what performance means when it comes to media playback... As its name suggest, this is not a generic media playback performance test, but rather it tries to estimate the composition performance changes. As it happens with many tests, it's sometimes hard to completely and fully isolate the main test subject, and so the tests numbers are many times affected by factors which are seemingly not meant to be measured. This is OK for two main reasons: - The test numbers are not some divine thing which we worship. If the numbers change, we try to understand what caused the change, and assess whether or not it's expected, whether or not it's worth further examination, etc. Often we just accept it because it really fixes an issue which has some performance tradeoffs, or because there are improvements elsewhere which did not get measured, etc. Sometimes it does alert the developer to some unexpected consequence. The goal is to prevent unintended regressions from slipping between the cracks. - The fact that the test doesn't focus on those other factors doesn't mean they can't cause real regressions where the test does focus. It always comes down to a human assessing whether or not the change alert is useful and/or meaningful. At this case, the apparent improvements seem to be a false negative - by your assessment. That's fine. > to me this is only further proof that talos indication is nonsensical in the > context of media playback and it's just measuring the wrong thing. A lot of effort has gone and is going into making sure that we're measuring the correct things at the talos tests. Specifically for this test, there have been a lot of discussions about it at bug 1254898 - where this test was written. Any improvements suggestions are highly welcome. FWIW, I'm still not convinced that this test measures the wrong thing. The numbers do reflect other things outside of its main goal, but as mentioned earlier, that's not necessarily a bad thing. Are there other tests you refer to "in the context of media playback" (or otherwise) which we should re-examine? (In reply to Jean-Yves Avenard [:jya] from comment #83) > You can't just time how long the compositor takes to render a particular > frame. > You are measuring how long it takes to demux a sample, decode the frame, > wait until it's time to display that sample, and then the sample to be > rendered. That is correct. However, typically it's not hard to assess where does the change in numbers come from. We're able to point at a specific commit, and then it's possible to assess whether or not the change is meaningful and/or whether or not we should care. Note that it was clear at the time the test was written that other factors affect it, and some efforts have been made to reduce their impact, such as choosing clips which are relatively easy to decode. As always, improvement suggestions are welcome. At this point in time, the test represents our best collective efforts to measure an important thing which we want to be able to track. Improvement suggestions are welcome.