Closed Bug 1297265 Opened 8 years ago Closed 8 years ago

Remove decode ahead logic and rework InputExhausted

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(12 files, 10 obsolete files)

Bug 1297265: P11. Rework WMF Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P1. Remove decode ahead logic.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P2. Amend MediaDataDecoder documentation to emphasize the new expected behavior.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P3. Rework Apple VT use of InputExhausted.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1297265: P4. Rework Apple AudioToolbox use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P5. Rework Blank Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P6. Rework Opus Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P7. Rework Theora Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P8. Rework LibVPX Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P9. Rework Vorbis Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P10. Rework FFmpeg Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
u480271
: review+
Details
Bug 1297265: P12. Rework WAV Decoder use of InputExhausted.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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.
Priority: P3 → P1
No longer blocks: 1287379
Attachment #8786212 - Flags: review?(cpearce) → review?(jyavenard)
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
Attachment #8786212 - Flags: review?(jyavenard) → review+
Attachment #8786210 - Attachment is obsolete: true
Attachment #8786210 - Flags: review?(dglastonbury)
Attachment #8786211 - Attachment is obsolete: true
Attachment #8786211 - Flags: review?(dglastonbury)
Attachment #8786212 - Attachment is obsolete: true
Attachment #8786212 - Flags: review?(cpearce)
Attachment #8786213 - Attachment is obsolete: true
Attachment #8786213 - Flags: review?(dglastonbury)
Attachment #8786214 - Attachment is obsolete: true
Attachment #8786214 - Flags: review?(dglastonbury)
Attachment #8786215 - Attachment is obsolete: true
Attachment #8786215 - Flags: review?(dglastonbury)
Attachment #8786216 - Attachment is obsolete: true
Attachment #8786216 - Flags: review?(dglastonbury)
Attachment #8786217 - Attachment is obsolete: true
Attachment #8786217 - Flags: review?(dglastonbury)
Attachment #8786218 - Attachment is obsolete: true
Attachment #8786218 - Flags: review?(dglastonbury)
Attachment #8786219 - Attachment is obsolete: true
Attachment #8786219 - Flags: review?(dglastonbury)
Attachment #8786312 - Flags: review?(cpearce) → review?(jyavenard)
Comment on attachment 8786312 [details]
Bug 1297265: P3. Rework Apple VT use of InputExhausted.

https://reviewboard.mozilla.org/r/75288/#review73112
Attachment #8786312 - Flags: review?(jyavenard) → review+
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
Attachment #8786310 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786311 [details]
Bug 1297265: P2. Amend MediaDataDecoder documentation to emphasize the new expected behavior.

https://reviewboard.mozilla.org/r/75286/#review73792
Attachment #8786311 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786313 [details]
Bug 1297265: P4. Rework Apple AudioToolbox  use of InputExhausted.

https://reviewboard.mozilla.org/r/75290/#review73794
Attachment #8786313 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786314 [details]
Bug 1297265: P5. Rework Blank Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75292/#review73796
Attachment #8786314 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786315 [details]
Bug 1297265: P6. Rework Opus Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75294/#review73798
Attachment #8786315 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786316 [details]
Bug 1297265: P7. Rework Theora Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75296/#review73800
Attachment #8786316 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786317 [details]
Bug 1297265: P8. Rework LibVPX Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75298/#review73802
Attachment #8786317 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786318 [details]
Bug 1297265: P9. Rework Vorbis Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75300/#review73804
Attachment #8786318 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786319 [details]
Bug 1297265: P10. Rework FFmpeg Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75302/#review73806
Attachment #8786319 - Flags: review?(dglastonbury) → review+
Comment on attachment 8786220 [details]
Bug 1297265: P11. Rework WMF Decoder use of InputExhausted.

https://reviewboard.mozilla.org/r/75216/#review73808
Attachment #8786220 - Flags: review?(dglastonbury) → review+
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
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
Attachment #8787139 - Flags: review?(gsquelart) → review+
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
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 :)
Flags: needinfo?(ethlin)
(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.
Flags: needinfo?(ethlin) → needinfo?(jyavenard)
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)
Flags: needinfo?(jyavenard)
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.
Depends on: 1296211
Depends on: 1301068
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: