1033913 - Integrate asynchronous decoding mechanism into MediaCodecReader.

Status: RESOLVED INVALID

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

Description

[Bruce Sun [:brsun]]

This is a followup bug of bug 904177. Use asynchronous mechanism to decode audio data and video data.

Comment 1

[Ivan Tsay (:ITsay)]

This is part of 2.1 feature work on Async codec.

Comment 2

[Benjamin Chen [:bechen]]

Remove 2.1 feature flag because bug 1033912 covers the most important parts of asynchronous decoding.
Leave this bug as an enhancement.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: bug-1033913.v01.patch

It's a huge patch, because I change the threading model a lot.

1. Change |mTaskQueue| to |mInputTaskQueue and mOutputTaskQueue| for both audio/video.
2. FillCodecInputData runs on mInputTaskQueue; GetCodecOutputData runs on mOutputTaskQueue.
3. A new looper thread mActivityLooper: listen the MediaCodec activity change and dispatch an input task.
4. Refine the ReleaseCriticalResources/ResetDecode functions in order to avoid some crash/resource leakage.
5. Seek function need to sync all the threads, reuse the |mDurationLock| to protect some variables accessed by multi-threads.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note there have been some bugs found in async MediaDecoderReader in bug 1032633. You might want to apply the patch in bug 1032633 when you test this patch.

Comment 5

[Bruce Sun [:brsun]]

Comment on attachment 8521958 [details] [diff] [review]
bug-1033913.v01.patch

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

Basically it looks good. Some ideas are commented inline.

::: dom/media/omx/MediaCodecReader.cpp
@@ +361,3 @@
>      RefPtr<nsIRunnable> task =
>        NS_NewRunnableMethod(this,
>                             &MediaCodecReader::DecodeAudioDataTask);

nit: It would be clearer if we could align the naming between filling input data (FillAudioInputTask) and getting output data (DecodeAudioDataTask) like GetAudioOutputTask or something.

@@ +372,4 @@
>      RefPtr<nsIRunnable> task =
>        NS_NewRunnableMethodWithArg<int64_t>(this,
>                                             &MediaCodecReader::DecodeVideoFrameTask,
>                                             aTimeThreshold);

nit: Ditto for DecodeVideoFrameTask.

@@ +1029,5 @@
>    MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>  
> +  DestroyActivityLooper();
> +  MutexAutoLock videoLock(mVideoTrack.mDurationLock);
> +  MutexAutoLock audioLock(mAudioTrack.mDurationLock);

Holding more than one lock at the same time is a little dangerous. The locking/unlocking sequences need to be handled carefully, and it tends to deadlock if some modifications made by others in the future. We could try to avoid that situation by limiting the scope of each lock to the extreme.

@@ +1086,5 @@
> +
> +  CreateActivityLooper();
> +  if (mAudioTrack.mCodec != nullptr) {
> +    mActivityAudioMessage = new AMessage(kNotifyAudioCodecUpdate,
> +                                         mActivityHandler->id());

mActivityAudioMessage seems doubly created. Although there is no resource leakage problem, probably it could be more efficient. Maybe the creation of AMessage could be moved into some earlier stage for reuse.

@@ +1091,5 @@
> +    mAudioTrack.mCodec->requestActivityNotification(mActivityAudioMessage->dup());
> +  }
> +  if (mVideoTrack.mCodec != nullptr) {
> +    mActivityVideoMessage = new AMessage(kNotifyVideoCodecUpdate,
> +                                         mActivityHandler->id());

Ditto for mActivityVideoMessage.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

MediaCodecReader has been removed, so closing bug.