Closed Bug 1111966 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jya, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
This looks like a regression from the async mp4 patches.
Assignee: nobody → matt.woodrow
Attachment #8537050 - Flags: review?(cpearce)
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...
(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 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) ...
Attachment #8537050 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/1853501bdde1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8537050 - Flags: approval-mozilla-aurora?
Attachment #8537050 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.