Closed Bug 1043274 Opened 7 years ago Closed 7 years ago

[FireFox OS] Use GraphicBuffer on GonkDecoderModule

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.2 S1 (5dec)
tracking-b2g backlog

People

(Reporter: bwu, Assigned: bwu)

References

Details

(Whiteboard: [joint-triage+])

Attachments

(1 file, 2 obsolete files)

This is a follow-up bug for bug 941302. 
It would be better to make render pipeline use graphicBuffer to avoid memory copy and color conversion for the decoded frame from MediaCodecProxy. 
Bug 1009410 and bug 1009420 are for the discussion about how to get graphicBuffer from MediaCodec or is there anyway to get it.
Depends on: 941302
Depends on: 1009410
Assignee: nobody → bwu
feature-b2g: --- → 2.1
Target Milestone: --- → 2.1 S3 (29aug)
[Blocking Requested - why for this release]:

Per offline discussion with Ken Chang, Marvin Khoo and Blake Wu, I'm going to move this from 2.1 feat+ to 2.1?. In addition, nominate this as 2.1+ since it will largely improve the performance.
blocking-b2g: --- → 2.1?
feature-b2g: 2.1 → ---
It is a 2.1+ bug.
blocking-b2g: 2.1? → 2.1+
[Blocking Requested - why for this release]:

Hi all! This is a feature bug, correctly identified by Kevin previously. Performance improvements shouldn't block an entire release - instead are taken when ready.

See the blocker guidelines for reference on what blocks and doesn't:

https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines

Ken, if it meets the criteria, please re-set the flag. Thanks!
blocking-b2g: 2.1+ → 2.1?
Flags: needinfo?(kchang)
I would say it is a **Major issue in new feature - especially those in which a large number of users will be impacted**.
blocking-b2g: 2.1? → 2.1+
Flags: needinfo?(kchang)
Whiteboard: [joint-triage+]
Depends on: 1039182
Any update for this bug?
Flags: needinfo?(bwu)
[Blocking Requested - why for this release]:
Since this bug is for MSE (bug 1054207) and it is not 2.1 feature, we should remove it from 2.1+.
blocking-b2g: 2.1+ → 2.1?
Flags: needinfo?(bwu)
Okay, moving to backlog per comment 6
blocking-b2g: 2.1? → backlog
(In reply to Blake Wu [:bwu][:blakewu] from comment #6)
> [Blocking Requested - why for this release]:
> Since this bug is for MSE (bug 1054207) and it is not 2.1 feature, we should
Oops! one typo. The MSE bug is Bug 1027519.  
> remove it from 2.1+.
Blocks: MSE-FxOS
Summary: [FireFox OS] Use GraphicBuffer on PlatformDecoderModule → [FireFox OS] Use GraphicBuffer on GonkDecoderModule
Target Milestone: 2.1 S3 (29aug) → 2.2 S1 (5dec)
Attachment #8520491 - Flags: review?(sotaro.ikeda.g)
Attachment #8520491 - Flags: review?(edwin)
Comment on attachment 8520491 [details] [diff] [review]
Bug-1043274-Use-GraphicBuffer-on-GonkDecoderModule.patch

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

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ -98,4 @@
>    }
>    // Create ALooper
>    mLooper = new ALooper;
> -  mLooper->setName("GonkVideoDecoderManager");

Should mLooper still have a name?

@@ +184,5 @@
> +                          keyFrame,
> +                          -1,
> +                          picture);
> +
> +  } else {

Check mVideoBuffer->data() for null here.

@@ +537,5 @@
> +  videoManager->PostReleaseVideoBuffer(client->GetMediaBuffer(), client->GetReleaseFenceHandle());
> +}
> +
> +void GonkVideoDecoderManager::PostReleaseVideoBuffer(android::MediaBuffer *aBuffer,
> +                                             const FenceHandle& aReleaseFenceHandle)

nit: indent
Attachment #8520491 - Flags: review?(edwin) → review+
Thanks for review. 
(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> Comment on attachment 8520491 [details] [diff] [review]
> Bug-1043274-Use-GraphicBuffer-on-GonkDecoderModule.patch
> 
> Review of attachment 8520491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ -98,4 @@
> >    }
> >    // Create ALooper
> >    mLooper = new ALooper;
> > -  mLooper->setName("GonkVideoDecoderManager");
> 
> Should mLooper still have a name?
This name is not a must and may be overwrote by MediaCodec since mLooper will be passed to MediaCodec. 
> 
> @@ +184,5 @@
> > +                          keyFrame,
> > +                          -1,
> > +                          picture);
> > +
> > +  } else {
> 
> Check mVideoBuffer->data() for null here.
Will do! 
> 
> @@ +537,5 @@
> > +  videoManager->PostReleaseVideoBuffer(client->GetMediaBuffer(), client->GetReleaseFenceHandle());
> > +}
> > +
> > +void GonkVideoDecoderManager::PostReleaseVideoBuffer(android::MediaBuffer *aBuffer,
> > +                                             const FenceHandle& aReleaseFenceHandle)
> 
> nit: indent
Will do!
Comment on attachment 8520491 [details] [diff] [review]
Bug-1043274-Use-GraphicBuffer-on-GonkDecoderModule.patch

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

Fence handling code does not work at all for MediaCodec. Its code could cause status inconsistency between MediaCodec and GonkNativeWindow. It seems better to remove the related part and fix the fence problem in a different bug like Bug 1091467.

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +552,5 @@
> +
> +}
> +
> +void GonkVideoDecoderManager::ReleaseAllPendingVideoBuffersLocked()
> +{

This fence handling trick works only on OMXCodec. On MediaCodec, it does not work. I already commented same thing at Bug 1033903 comment 17. And Bug 1091467 seems to be created to handle the problem.
Attachment #8520491 - Flags: review?(sotaro.ikeda.g)
Thanks for your review! 
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Comment on attachment 8520491 [details] [diff] [review]
> Bug-1043274-Use-GraphicBuffer-on-GonkDecoderModule.patch
> 
> Review of attachment 8520491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fence handling code does not work at all for MediaCodec. Its code could
> cause status inconsistency between MediaCodec and GonkNativeWindow. It seems
> better to remove the related part and fix the fence problem in a different
> bug like Bug 1091467.
> 
> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ +552,5 @@
> > +
> > +}
> > +
> > +void GonkVideoDecoderManager::ReleaseAllPendingVideoBuffersLocked()
> > +{
> 
> This fence handling trick works only on OMXCodec. On MediaCodec, it does not
> work. I already commented same thing at Bug 1033903 comment 17. And Bug
> 1091467 seems to be created to handle the problem.
Got it. I will remove the related codes and create a follow up bug for it.
Some changes are made according to comment 10 and comment 12.
Attachment #8520491 - Attachment is obsolete: true
Attachment #8521254 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8521254 [details] [diff] [review]
Bug-1043274-FireFox-OS-Use-GraphicBuffer-on-GonkDeco-v2.patch

Looks good! Please do not forget to address fence handling problem:-)
Attachment #8521254 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> Comment on attachment 8521254 [details] [diff] [review]
> Bug-1043274-FireFox-OS-Use-GraphicBuffer-on-GonkDeco-v2.patch
> 
> Looks good! Please do not forget to address fence handling problem:-)
Oops. Forgot to post the follow-up bug here. Thanks for reminding me! I created the Bug 1097498 to apply fence.
Carry r+ from sotaro and edwin.
Attachment #8521254 - Attachment is obsolete: true
Attachment #8522021 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2f9619594cf0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.