Closed Bug 1185931 Opened 4 years ago Closed 4 years ago

Replace |if (mDecoderStateMachine)| by |MOZ_ASSERT(mDecoderStateMachine)| in MediaDecoder.cpp.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox42 --- affected
firefox47 --- fixed

People

(Reporter: bechen, Assigned: bechen)

Details

Attachments

(1 file, 2 obsolete files)

Fork from bug 1183394 comment 9.
Component: Audio/Video → Audio/Video: Playback
Do you still care about this bug?
Flags: needinfo?(bechen)
Assignee: nobody → bechen
Flags: needinfo?(bechen)
Attached patch bug-1185931.patch (obsolete) — Splinter Review
Finally I only add one line code after review the |mDecoderStateMachine| and |GetStateMachine()|.
Attachment #8721181 - Flags: review?(jwwang)
Comment on attachment 8721181 [details] [diff] [review]
bug-1185931.patch

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

Can you explain the purpose of this change?
(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8721181 [details] [diff] [review]
> bug-1185931.patch
> 
> Review of attachment 8721181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you explain the purpose of this change?

|MediaOmxCommonDecoder::PauseStateMachine| only called at |MediaOmxCommonDecoder::FirstFrameLoaded|. And the FirstFrameLoaded event comes from |MediaDecoderStateMachine::EnqueueFirstFrameLoadedEvent|.
So if we hit the assertion, means that the MDSM just fire the FirstFrameLoaded event then be destroyed quickly, we might need to care about it.
MediaDecoder::FirstFrameLoaded() asserts |!mShuttingDown|. I think you can also remove the if statement.
Attached patch bug-1185931.patch (obsolete) — Splinter Review
Address comment 5.
Attachment #8721181 - Attachment is obsolete: true
Attachment #8721181 - Flags: review?(jwwang)
Attachment #8722906 - Flags: review?(jwwang)
Comment on attachment 8722906 [details] [diff] [review]
bug-1185931.patch

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

::: dom/media/omx/MediaOmxCommonDecoder.cpp
@@ +131,1 @@
>    if (!GetStateMachine()) {

If you assert |GetStateMachine()|, you don't need to test it here.
Attachment #8722906 - Flags: review?(jwwang)
Address comment 7.
Attachment #8722906 - Attachment is obsolete: true
Attachment #8723421 - Flags: review?(jwwang)
Attachment #8723421 - Flags: review?(jwwang) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c5033cf8574
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.