Closed
Bug 1043274
Opened 10 years ago
Closed 10 years ago
[FireFox OS] Use GraphicBuffer on GonkDecoderModule
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: bwu, Assigned: bwu)
References
Details
(Whiteboard: [joint-triage+])
Attachments
(1 file, 2 obsolete files)
22.80 KB,
patch
|
bwu
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bwu
Updated•10 years ago
|
feature-b2g: --- → 2.1
Target Milestone: --- → 2.1 S3 (29aug)
Comment 1•10 years ago
|
||
[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 → ---
Comment 3•10 years ago
|
||
[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)
Comment 4•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [joint-triage+]
Assignee | ||
Comment 6•10 years ago
|
||
[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)
Assignee | ||
Comment 8•10 years ago
|
||
(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+.
Assignee | ||
Updated•10 years ago
|
Summary: [FireFox OS] Use GraphicBuffer on PlatformDecoderModule → [FireFox OS] Use GraphicBuffer on GonkDecoderModule
Target Milestone: 2.1 S3 (29aug) → 2.2 S1 (5dec)
Assignee | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
Carry r+ from sotaro and edwin.
Attachment #8521254 -
Attachment is obsolete: true
Attachment #8522021 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9b1a717791a3 Test results look good!
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9619594cf0
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f9619594cf0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•