1168778 - MediaCodecReader: crash when seek.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

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));

Comment 1

[Bobby Holley (:bholley)]

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?

Comment 2

[Benjamin Chen [:bechen]]

Attachment: bug-1168778.v01.patch

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

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().

Comment 4

[Benjamin Chen [:bechen]]

(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().

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1168778.v02.patch

Release the monitor when call DecodeVideoFrameSync/DecodeAudioDataSync and assertion for DecodeAudioDataTask/DecodeVideoFrameTask.

Comment 6

[Sotaro Ikeda [:sotaro]]

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.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: bug-1168778.v02.patch

r=sotaro, add some comments.
try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f38fbbf854

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/141c6315de6a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/141c6315de6a