911548 - Add necessary patches for aosp gonk stagefright

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

aosp's OMXCodec do not use allocated output buffers, return the last two buffers to the native window. In android, ANativeWindow is used for buffer source and rendering target by OMXCodec. The return buffers are necessary not to block the rendering target's task. But in b2g, ANativeWindow is used just for buffer source, do not need to return the buffers.

The change is done in ICS caf in Bug 864230 for v1.x. But for nexus-4, we are using JB aosp as android source. So we need to apply the change for nexus-4.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: path - Change OMXCodec to use all allocated output buffers

Comment 2

[Sotaro Ikeda [:sotaro]]

This is necessary only on AOSP.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: pull request to gonk-patches

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: pull request to b2g-manifest

Comment 6

[Sotaro Ikeda [:sotaro]]

b2g nexus-4 uses aosp as base gonk code, not caf. We need to maintain necessary change to gonk. This bug is for stagefright change.

Comment 7

[Sotaro Ikeda [:sotaro]]

Bug 871018 is also important change in b2g18. But it is not necessary for AOSP. Bug 871018 is necessary only on caf's OMXCodec.

Comment 8

[Sotaro Ikeda [:sotaro]]

b2g18 has another patch related to stagefright. It is Bug 823324. I feel it is not ncecessary even in caf b2g18. It is added to mitigate the incorrect gralloc buffer handling crash. But this patch derive another problem. If there is the problem, hw codec is not freed forever. It make situation worse.

Comment 9

[Michael Wu [:mwu]]

gonk-patches will not be used for post-ICS devices. We should fork frameworks/av instead.

Comment 10

[Michael Wu [:mwu]]

I've forked frameworks/av. Use https://github.com/mozilla-b2g/platform_frameworks_av/tree/b2g-4.3_r2.1

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: pull request to mozilla-b2g/platform_frameworks_av

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - change to b2g-manifest

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 802614 [details] [diff] [review]
patch - change to b2g-manifest

I'll make this change in-place (instead of moving it to the B2G section).

Comment 14

[Michael Wu [:mwu]]

https://github.com/mozilla-b2g/b2g-manifest/commit/6d392e292cefa0167d288cba3218e69db8bac689

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> Created attachment 802607 [details]
> pull request to mozilla-b2g/platform_frameworks_av

The change of the color conversion comes from codeaurora.
https://www.codeaurora.org/cgit/external/simcom/platform/frameworks/av/tree/media/libstagefright/colorconversion/ColorConverter.cpp?h=jb-8930-master-dsda

See Bug 911518 comment #2.

Comment 16

[Michael Wu [:mwu]]

I'm still not comfortable with the use of libmm-color-convertor.so. I don't see any evidence that libmm-color-convertor.so uses anything other than software running on the cpu to do its job. This particular format has also been reverse engineered.

Comment 17

[Sotaro Ikeda [:sotaro]]

As I told in the oslo b2g work week, caf did color conversion by software in the past. I found the following code. If it is not for the product, it seems enough. How do you think about the code? 

https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/media/libstagefright/colorconversion/ColorConverter.cpp?id=M8960AAAAANLYA1001

Comment 18

[Michael Wu [:mwu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> As I told in the oslo b2g work week, caf did color conversion by software in
> the past. I found the following code. If it is not for the product, it seems
> enough. How do you think about the code? 
> 
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/media/
> libstagefright/colorconversion/ColorConverter.cpp?id=M8960AAAAANLYA1001

Looks reasonable. If we can build that code in instead of dlopening at runtime, I'll be ok with this.

Comment 19

[Sotaro Ikeda [:sotaro]]

Thanks for the comment. I am going to implement the patch by using the software color conversion.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: pull request to mozilla-b2g/platform_frameworks_av

Comment 21

[Sotaro Ikeda [:sotaro]]

Comment on attachment 809228 [details]
pull request to mozilla-b2g/platform_frameworks_av

mwu, can you review the pull request. I confirmed the patch work on nexus-4 with temporary modification on Bug 910498.

Comment 22

[Michael Wu [:mwu]]

Comment on attachment 809228 [details]
pull request to mozilla-b2g/platform_frameworks_av

Much better - thanks!

I didn't do an in-depth review since I assume this is mostly just code imported from other places.

Didn't you also have another patch to frameworks/av/media/libstagefright/OMXCodec.cpp ? r=me for that too if you still want to land that.

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #22)
> 
> Didn't you also have another patch to
> frameworks/av/media/libstagefright/OMXCodec.cpp ? r=me for that too if you
> still want to land that.

Thanks for the quick review! The change to OMXCodec.cpp is already included in the pull request. It is only one line change.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> (In reply to Michael Wu [:mwu] from comment #22)
> > 
> > Didn't you also have another patch to
> > frameworks/av/media/libstagefright/OMXCodec.cpp ? r=me for that too if you
> > still want to land that.
> 
> Thanks for the quick review! The change to OMXCodec.cpp is already included
> in the pull request. It is only one line change.

It's the following part.

--------------------------------------
         cancelEnd = mPortBuffers[kPortIndexOutput].size();
     } else {
         // Return the last two buffers to the native window.
-        cancelStart = def.nBufferCountActual - minUndequeuedBufs;
+        cancelStart = def.nBufferCountActual;
         cancelEnd = def.nBufferCountActual;
     }

Comment 25

[Michael Wu [:mwu]]

Ah, I missed it.

Actually, can you pull that change out into its own commit? It makes sense on its own.

You should have write access to the repo so commit whenever you're ready.

Comment 26

[Sotaro Ikeda [:sotaro]]

Okey, I am going to split the change to own commit.

Comment 27

[Sotaro Ikeda [:sotaro]]

I split the change to 2 commit and merged.
https://github.com/mozilla-b2g/platform_frameworks_av/commit/b86fa5f734f3976c52ca15bfbea803bc945230cc
https://github.com/mozilla-b2g/platform_frameworks_av/commit/ec46d95f9d369ec883768f8617224b640cf073cb

Comment 28

[Sotaro Ikeda [:sotaro]]

Oh, I need to uplift the change to "b2g-4.3_r2.1".

Comment 29

[Michael Wu [:mwu]]

Yeah - sorry I missed that. Please reset the master branch to its previous state after uplifting.

Comment 30

[Sotaro Ikeda [:sotaro]]

Uplift the change to "b2g-4.3_r2.1".

https://github.com/mozilla-b2g/platform_frameworks_av/commit/29e73ecbf09a29114862a62dd3d9b3c5749dc7fb
https://github.com/mozilla-b2g/platform_frameworks_av/commit/7c4fbe57bd8d2581ca729f044b5b8324710d5fe8

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #29)
> Yeah - sorry I missed that. Please reset the master branch to its previous
> state after uplifting.

Okey.

Comment 32

[Sotaro Ikeda [:sotaro]]

reset of mater is completed. mwu, thanks for your help.

Comment 33

[Inder]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Comment on attachment 809228 [details]
> pull request to mozilla-b2g/platform_frameworks_av
> 
> mwu, can you review the pull request. I confirmed the patch work on nexus-4
> with temporary modification on Bug 910498.

Sotaro, can you please explain how you came up with the color conversion changes here https://github.com/sotaroikeda/platform_frameworks_av/commit/048364d2e6d0ee71f83447d3fc9cf1fab1815ae1?
Correct me if I am wrong but I assume all these color conversion changes already exist on CAF and we don't need to make any corresponding changes.

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Inder from comment #33)
> (In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Sotaro, can you please explain how you came up with the color conversion
> changes here
> https://github.com/sotaroikeda/platform_frameworks_av/commit/
> 048364d2e6d0ee71f83447d3fc9cf1fab1815ae1?
> Correct me if I am wrong but I assume all these color conversion changes
> already exist on CAF and we don't need to make any corresponding changes.

It is necessary, because current moz build for nexus 4 does not use caf, but use AOSP.

Comment 35

[Sotaro Ikeda [:sotaro]]

Inder, does caf provide the support for nexus4 build? And can you give me a link of corresponding color conversion code?

Comment 36

[Sotaro Ikeda [:sotaro]]

And I am not the person to make decision to use AOSP for nexus4 moz build.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Should this have POVB or NPOTB on the whiteboard? It's showing up in my needs-uplift queries, but it sure doesn't look like there's anything actionable here on my end.

Comment 38

[Sotaro Ikeda [:sotaro]]

I add POVB on the whiteborad.

Comment 39

[Inder]

(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> (In reply to Inder from comment #33)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #21)
> > Sotaro, can you please explain how you came up with the color conversion
> > changes here
> > https://github.com/sotaroikeda/platform_frameworks_av/commit/
> > 048364d2e6d0ee71f83447d3fc9cf1fab1815ae1?
> > Correct me if I am wrong but I assume all these color conversion changes
> > already exist on CAF and we don't need to make any corresponding changes.
> 
> It is necessary, because current moz build for nexus 4 does not use caf, but
> use AOSP.

Sotaro,
  Sorry, if I caused confusion. I am aware that nexus 4 uses AOSP and I wasn't suggesting you to start using CAF. I was just wondering if I need to make any corresponding changes in CAF. I noticed that in your commit you brought in CAF copyright so I guess your color conversion changes are from CAF somewhere?
See: https://github.com/mozilla-b2g/platform_frameworks_av/blob/b2g-4.3_r2.1/media/libstagefright/colorconversion/ColorConverter.cpp#L3

Comment 40

[Sotaro Ikeda [:sotaro]]

Yes, I borrowed the color conversion code from CAF. The first version of pull request was made from the following.
https://www.codeaurora.org/cgit/external/simcom/platform/frameworks/av/tree/media/libstagefright/colorconversion/ColorConverter.cpp?h=jb-8930-master-dsda

The second(final) version was made from the following. The following code is a bit old, but still seems to convert color correctly.
https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/media/libstagefright/colorconversion/ColorConverter.cpp?id=M8960AAAAANLYA1001

Comment 41

[Sotaro Ikeda [:sotaro]]

Hi Inder, in ics_1.2 "persist.camera.shutter.disable" is defined to skip loading sound filed in CameraService, but in b2g_jb_mr2 do not have this system property. Is CAF going to add the property to CAF b2g_jb?

https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/services/camera/libcameraservice/CameraService.cpp?h=b2g_ics_1.2#n297

https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/services/camera/libcameraservice/CameraService.cpp?h=b2g_jb_mr2

Comment 42

[Inder]

Sotaro -- we didn't have plans to add the system property. Do we need this for proper shutter sound functioning?

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/
> services/camera/libcameraservice/CameraService.cpp?h=b2g_ics_1.2#n297

I asked the question just because codeaurora added the system property to CameraService on android ICS.

It is not necessary for the shutter sound functioning on b2g. On b2g, we disable CameraService's shutter sound just by removing android default sound files. But it causes spend several ms to try to load the files by using android::MediaPlayer. The above disable system shutter sound at all. The CameraService does not even try to load the file.