1111966 - MSE/MP4: Calculation of parsed/decoded frames is wrong

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

The amount of frames reported as dropped is often wrong. Sometimes with a value > 4,000,000,000.

since MP4 decoding is now asynchronous; we shouldn't perform the calculations on how many frames have been decoded or parsed in MP4Reader::RequestVideoData

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Report frame counts correctly

This looks like a regression from the async mp4 patches.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8537050 [details] [diff] [review]
Report frame counts correctly

Review of attachment 8537050 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/MP4Reader.cpp
@@ +585,5 @@
> +  // Record number of frames decoded and parsed. Automatically update the
> +  // stats counters using the AutoNotifyDecoded stack-based class.
> +  uint32_t parsed = 0, decoded = 0;
> +  AbstractMediaDecoder::AutoNotifyDecoded autoNotify(mDecoder, parsed, decoded);
> +

Shouldn't this be done only for aTrack == kVideo ?

can't use AutoNotifyDecoded I'm afraid.

so have to call mDecoder->NotifyDecodedFrames manually

@@ +626,5 @@
>    if (needInput) {
>      MP4Sample* sample = PopSample(aTrack);
>      if (sample) {
>        decoder.mDecoder->Input(sample);
> +      parsed++;

same here...

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Shouldn't this be done only for aTrack == kVideo ?
> 
> can't use AutoNotifyDecoded I'm afraid.
> 
> so have to call mDecoder->NotifyDecodedFrames manually

I don't think this overly matters, since we'll just report 0's when it's audio.

I figured it was cleaner this way, but I don't mind changing it.

> 
> @@ +626,5 @@
> >    if (needInput) {
> >      MP4Sample* sample = PopSample(aTrack);
> >      if (sample) {
> >        decoder.mDecoder->Input(sample);
> > +      parsed++;
> 
> same here...

Yes, good catch.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8537050 [details] [diff] [review]
Report frame counts correctly

Review of attachment 8537050 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/MP4Reader.cpp
@@ +584,5 @@
>  
> +  // Record number of frames decoded and parsed. Automatically update the
> +  // stats counters using the AutoNotifyDecoded stack-based class.
> +  uint32_t parsed = 0, decoded = 0;
> +  AbstractMediaDecoder::AutoNotifyDecoded autoNotify(mDecoder, parsed, decoded);

The function AutoNotifyDecoded calls in its dtor bails if (parsed==0 and decoded==0), so this is ok.

@@ +601,5 @@
> +    if (aTrack == kVideo) {
> +      uint64_t delta = decoder.mNumSamplesOutput - mLastReportedNumDecodedFrames;
> +      decoded = static_cast<uint32_t>(delta);
> +      mLastReportedNumDecodedFrames = decoder.mNumSamplesOutput;
> +

Nit: extra blank line here.

@@ +626,5 @@
>    if (needInput) {
>      MP4Sample* sample = PopSample(aTrack);
>      if (sample) {
>        decoder.mDecoder->Input(sample);
> +      parsed++;

if (aTrack == kVideo) ...

Comment 5

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1853501bdde1

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1853501bdde1

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8537050 [details] [diff] [review]
Report frame counts correctly

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve Flash video.
[Describe test coverage new/current, TBPL]: landed on m-c.
[Risks and why]: Low. This affects MSE-specific code.
[String/UUID change made/needed]: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/233190826841