Closed
Bug 1111966
Opened 9 years ago
Closed 9 years ago
MSE/MP4: Calculation of parsed/decoded frames is wrong
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jya, Assigned: mattwoodrow)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
4.01 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
This looks like a regression from the async mp4 patches.
Assignee: nobody → matt.woodrow
Attachment #8537050 -
Flags: review?(cpearce)
Reporter | ||
Comment 2•9 years ago
|
||
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...
Assignee | ||
Comment 3•9 years ago
|
||
(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•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1853501bdde1
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1853501bdde1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 7•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
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.
Description
•