MediaCodecReader: crash when seek.

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

MediaCodecReader crash when seek.

https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecReader.cpp?from=mediacodecreader.cpp#705

https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecReader.cpp?from=mediacodecreader.cpp#859


Dispatch (this=<optimized out>, aRunnable=..., aFailureHandling=<optimized out>, aReason=<optimized out>) at /home/benjamin/Documents/hg/mozilla-central/dom/media/MediaTaskQueue.h:53
53	    MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));
If the task queue is flushable, you probably need to pass DontAssertDispatchSuccess to Dispatch, since we reject new tasks while flushing. We're trying to move away from flushing though - is there any way we can get rid of it here?
Posted patch bug-1168778.v01.patch (obsolete) — Splinter Review
1. Replace the FlushableMediaTaskQueue by MediaTaskQueue.
2. Because there is no flush funcion for the TaskQueue, so I need to refact the seek/DecodeAudioDataTask/DecodeVideoFrameTask functions. Seek() and ResetDecode() funcions run at the same thread, concurrently DecodeAudioDataTask() and DecodeVideoFrameTask() run on our own taskqueues.
Attachment #8615902 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8615902 [details] [diff] [review]
bug-1168778.v01.patch

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

The code seems to work. But DecodeVideoFrameSync() and DecodeAudioDataSync() moved to within m***Track.mTrackMonitor, 2 functions could block longer time, therefore it seems not good because mTrackMonitor could be held longer time. It could block also block MediaCodecReader's task queue.

And it seems better to add asserting IsCurrentThreadIn().
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Comment on attachment 8615902 [details] [diff] [review]
> bug-1168778.v01.patch
> 
> Review of attachment 8615902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code seems to work. But DecodeVideoFrameSync() and DecodeAudioDataSync()
> moved to within m***Track.mTrackMonitor, 2 functions could block longer
> time, therefore it seems not good because mTrackMonitor could be held longer
> time. It could block also block MediaCodecReader's task queue.
> 
> And it seems better to add asserting IsCurrentThreadIn().

I'll try to write a patch to not hold the mTrackMonitor when call DecodeVideoFrameSync()/DecodeAudioDataSync().
Posted patch bug-1168778.v02.patch (obsolete) — Splinter Review
Release the monitor when call DecodeVideoFrameSync/DecodeAudioDataSync and assertion for DecodeAudioDataTask/DecodeVideoFrameTask.
Attachment #8615902 - Attachment is obsolete: true
Attachment #8615902 - Flags: review?(sotaro.ikeda.g)
Attachment #8617180 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8617180 [details] [diff] [review]
bug-1168778.v02.patch

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

Looks good. But implementation is a bit tricky. When DecodeVideoFrameSync() or DecodeAudioDataSync() is called, promise for the track always exists. But when exiting the function, there could be a case that the promise is cleared. The patch handling this case. It seems better to add comment about it.
Attachment #8617180 - Flags: review?(sotaro.ikeda.g) → review+
r=sotaro, add some comments.
try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f38fbbf854
Attachment #8617180 - Attachment is obsolete: true
Attachment #8617812 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/141c6315de6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.