1039182 - Add getGrapicBuffer interface to MediaCodecProxy

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Blake Wu [:bwu][:blakewu]]

Add an interface, getOutputGraphicBufferFromIndex, to get GraphicBuffer from MediaCodec. 
This is currently blocked by bug 1009410.

Comment 1

[Blake Wu [:bwu][:blakewu]]

Attachment: 0001-Bug-1039182-Add-getGrapicBuffer-interface-to-MediaCo.patch

Add getOutputGraphicBufferFromIndex() to get Graphic Buffer from MediaCodec. 
This needs to be called right after calling dequeueOutputBuffer().

Comment 2

[Ivan Tsay (:ITsay)]

This is part of 2.1 feature work on Async codec.

Comment 3

[Eric Chou [:ericchou] [:echou]]

[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? and nominate this as 2.1+ since it will largely improve the performance.

Comment 4

[Ken Chang[:kenkai]]

It is a 2.1+ bug.

Comment 5

[Dietrich Ayala (:dietrich)]

[Blocking Requested - why for this release]:

Hi Ken, Ivan and Eric! Can you explain why this should block the whole release?

Usually we do not block on performance improvements.

For reference, the reasons to block a release are listed at https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines.

Comment 6

[Bruce Sun [:brsun]]

Attachment: bug1039182_add_graphic_buffer_interface_into_media_codec_proxy.patch

Use SFINAE trick and template specialization trick to wrap interface-difference of MediaCodec. So that the correct OutputGraphicBufferStub template class can be chosen during compile time.

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8487040 [details] [diff] [review]
bug1039182_add_graphic_buffer_interface_into_media_codec_proxy.patch

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

Nice!

Comment 8

[Bruce Sun [:brsun]]

Attachment: bug1039182_add_graphic_buffer_interface_into_media_codec_proxy.checkin.patch

TBPL result seems good:
 - [default] media.omx.async.enabled pref-off: https://tbpl.mozilla.org/?tree=Try&rev=7b4aaa1ff606
 - media.omx.async.enabled pref-on: https://tbpl.mozilla.org/?tree=Try&rev=a2583a942188

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb5b760919a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4eb5b760919a

Comment 11

[Dietrich Ayala (:dietrich)]

Hi Eric, triage group is trying to make a determination on why this blocks. See comment #5 - we automatically block on perf *regressions* but perf wins usually ride the trains. Is there a reason we should block here?

Also, we're still in the approvals stage - so maybe instead of requesting blocking, you can ask for approval on the patch and land that way.

Comment 12

[Eric Chou [:ericchou] [:echou]]

(In reply to Dietrich Ayala (:dietrich) from comment #11)
> Hi Eric, triage group is trying to make a determination on why this blocks.
> See comment #5 - we automatically block on perf *regressions* but perf wins
> usually ride the trains. Is there a reason we should block here?
> 

Since this is not a perf regression, I'll remove the nomination (sorry for not noticing comment 5. You may want to ni me directly next time ;-) ). The reason we nominated this as a blocker was because we know that the patch could make a huge improvement on performance by using the new interface which has been created in bug 1009410.

> Also, we're still in the approvals stage - so maybe instead of requesting
> blocking, you can ask for approval on the patch and land that way.

Sure. Thanks Dietrich.