This is a follow-up bug of bug 1033903. MediaCodecReader should handle fence properly while using GraphicBuffer to carry decoded video contents.

After discuss with :jolin and :chung, we found Fence is a complicated problem.... And in this wip patch, I new a Fence() and set it into TextureClient's ReleaseFenceHandle before rendering, and then wait the fence at TextureClient's callback. The patch seems works (no flicking). But I'm not sure the fence is really works or not. Hi Sotaro: Do you know the HwComposer(layer/imagebridge) will signal the fence I new add? And do we(MediaCodecReader) need to handle the fence between HW decoder <--> HwComposer

Comment on attachment 8549435 [details] [diff] [review] bug-1091466.wip.patch Cancel the feedback request because I think my patch is wrong. Since I only new a Fence, there is no FD inside the fence.

chung mention that composing by GPU or HWcomposer might have different Fence implementation.

Thanks for John's help that we found the Fence is available in the textureClient callback. Any feedback is welcome.

Comment on attachment 8550224 [details] [diff] [review] bug-1091466.v01.patch Review of attachment 8550224 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/omx/MediaCodecReader.cpp @@ +809,5 @@ > + sp<Fence> fence = client->GetReleaseFenceHandle().mFence; > + if (fence.get() && fence->isValid()) { > + mPendingReleaseItems.AppendElement( > + ReleaseItem(index, client->GetReleaseFenceHandle())); > + } Here I check the fence first then append one element into mPendingReleaseItems. But some TextureClients doesn't have Fence inside (at the beginning of playing a stream), but we still need to release the outputBuffer by the index. I'll fix it at next version.

Comment on attachment 8550224 [details] [diff] [review] bug-1091466.v01.patch Review of attachment 8550224 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: dom/media/omx/MediaCodecReader.cpp @@ +804,5 @@ > if (!mTextureClientIndexes.Get(aClient, &index)) { > return; > } > + > + GrallocTextureClientOGL* client = static_cast<GrallocTextureClientOGL*>(aClient); Nit: type casting seems unnecessary because GetReleaseFenceHandle() is member function of TextureClient. @@ +808,5 @@ > + GrallocTextureClientOGL* client = static_cast<GrallocTextureClientOGL*>(aClient); > + sp<Fence> fence = client->GetReleaseFenceHandle().mFence; > + if (fence.get() && fence->isValid()) { > + mPendingReleaseItems.AppendElement( > + ReleaseItem(index, client->GetReleaseFenceHandle())); Nit: how about saving just mFence in ReleaseItem? It seems to me that's what you actually need anyway. ::: dom/media/omx/MediaCodecReader.h @@ +266,5 @@ > gfx::IntRect mRelativePictureRect; > // Protected by mTrackMonitor. > MediaPromiseHolder<VideoDataPromise> mVideoPromise; > > + nsRefPtr<MediaTaskQueue> mFenceTaskQueue; Nit: mReturnBufferTaskQueue or mReleaseBufferTaskQueue sounds more accurate to me. :) @@ +463,5 @@ > + } > + size_t mReleaseIndex; > + FenceHandle mReleaseFenceHandle; > + }; > + nsTArray<ReleaseItem> mPendingReleaseItems; Nit: how about adding some comments explaining what the fence is for?

Comment on attachment 8550224 [details] [diff] [review] bug-1091466.v01.patch Review of attachment 8550224 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Though I think MediaCodec should use BufferQueue ultimately. Otherwise, each new Android version change will bring similar issues.

(In reply to Chiajung Hung [:chiajung] from comment #7) > > Though I think MediaCodec should use BufferQueue ultimately. Otherwise, each > new Android version change will bring similar issues. We can not use same android's BufferQueue mechanism for fence handling of video playback. It is intentional thing to prevent a problem like Bug 1009410 Comment 21. Although WebRTC's MediaCodec usage is similar to android.

(In reply to Sotaro Ikeda [:sotaro] from comment #8) > (In reply to Chiajung Hung [:chiajung] from comment #7) > > > > Though I think MediaCodec should use BufferQueue ultimately. Otherwise, each > > new Android version change will bring similar issues. > > We can not use same android's BufferQueue mechanism for fence handling of > video playback. It is intentional thing to prevent a problem like Bug > 1009410 Comment 21. Although WebRTC's MediaCodec usage is similar to android. On android, stagefright dequeue only one video frame from MediaCodec for playback, but gecko need to get more video buffers. It causes the problem like Bug 1009410 Comment 21.

Comment on attachment 8550224 [details] [diff] [review] bug-1091466.v01.patch Review+ if Comment 5 is addressed.

(In reply to Sotaro Ikeda [:sotaro] from comment #9) > (In reply to Sotaro Ikeda [:sotaro] from comment #8) > > (In reply to Chiajung Hung [:chiajung] from comment #7) > > > > > > Though I think MediaCodec should use BufferQueue ultimately. Otherwise, each > > > new Android version change will bring similar issues. > > > > We can not use same android's BufferQueue mechanism for fence handling of > > video playback. It is intentional thing to prevent a problem like Bug > > 1009410 Comment 21. Although WebRTC's MediaCodec usage is similar to android. > It hard to tell what's wrong with Gecko from Bug 1009410 Comment 21. From [1], I believe the blocking issue in Bug 1009410 comes from slow consumer operation. For WebRTC(realtime) case, I would suggest to config BufferQueue to async mode and not blocking decoder. > On android, stagefright dequeue only one video frame from MediaCodec for > playback, but gecko need to get more video buffers. It causes the problem > like Bug 1009410 Comment 21. Why gecko need more? I think the output buffer amount requirement depends only on hardware configuration. Why gecko affect it? For layer system, the max acquire count is 1 as Android. (BufferQueue allows 2 acquire in [3] for acquire-then-release operation in [2], though). The difference seems to me is that Android SurfaceFlinger waits fence before release buffer, Gecko compositor sends the fence back. [1]http://androidxref.com/4.4_r1/xref/frameworks/av/media/libstagefright/ACodec.cpp#3436 [2]http://androidxref.com/4.4_r1/xref/frameworks/native/services/surfaceflinger/SurfaceFlingerConsumer.cpp#31 [3]http://androidxref.com/4.4_r1/xref/frameworks/native/libs/gui/BufferQueue.cpp#877

1. Address comment 5, comment 6. 2. r=sotaro 3. add "#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 17" try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c358be71032

Push to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33c95d115f87