Closed Bug 1091467 Opened 5 years ago Closed 5 years ago

MediaCodecReader should handle fence of GraphicBuffer properly.

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: brsun, Assigned: bechen)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a follow-up bug of bug 1033903.

MediaCodecReader should handle fence properly while using GraphicBuffer to carry decoded video contents.
Attached patch bug-1091466.wip.patch (obsolete) — Splinter Review
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
Attachment #8549435 - Flags: feedback?(sotaro.ikeda.g)
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.
Attachment #8549435 - Flags: feedback?(sotaro.ikeda.g)
chung mention that composing by GPU or HWcomposer might have different Fence implementation.
Attached patch bug-1091466.v01.patch (obsolete) — Splinter Review
Thanks for John's help that we found the Fence is available in the textureClient callback.

Any feedback is welcome.
Attachment #8549435 - Attachment is obsolete: true
Attachment #8550224 - Flags: review?(sotaro.ikeda.g)
Attachment #8550224 - Flags: feedback?(jolin)
Attachment #8550224 - Flags: feedback?(chung)
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?
Attachment #8550224 - Flags: feedback?(jolin) → feedback+
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.
Attachment #8550224 - Flags: feedback?(chung) → feedback+
(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.
Attachment #8550224 - Flags: review?(sotaro.ikeda.g) → review+
(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
Attachment #8550224 - Attachment is obsolete: true
Attachment #8551719 - Flags: review+
Duplicate of this bug: 1091466
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d8b23c23b131
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1097498
You need to log in before you can comment on or make changes to this bug.