Closed Bug 911548 Opened 9 years ago Closed 9 years ago

Add necessary patches for aosp gonk stagefright

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: POVB, QARegressExclude)

Attachments

(1 file, 5 obsolete files)

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.
Blocks: gonk-jb, 871364
blocking-b2g: --- → koi?
blocking-b2g: koi? → koi+
Assignee: nobody → sotaro.ikeda.g
This is necessary only on AOSP.
Duplicate of this bug: 911518
Summary: Change OMXCodec to use all allocated output buffers → Add necessary patches for aosp gonk stagefright
Attached file pull request to gonk-patches (obsolete) —
Attachment #798266 - Attachment is obsolete: true
Attached file pull request to b2g-manifest (obsolete) —
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.
Bug 871018 is also important change in b2g18. But it is not necessary for AOSP. Bug 871018 is necessary only on caf's OMXCodec.
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.
Attachment #801285 - Flags: review?(mwu)
Attachment #801287 - Flags: review?(mwu)
gonk-patches will not be used for post-ICS devices. We should fork frameworks/av instead.
Attachment #801285 - Attachment is obsolete: true
Attachment #801285 - Flags: review?(mwu)
Attachment #801287 - Attachment is obsolete: true
Attachment #801287 - Flags: review?(mwu)
Attached patch patch - change to b2g-manifest (obsolete) — Splinter Review
Attachment #802607 - Flags: review?(mwu)
Attachment #802614 - Flags: review?(mwu)
Depends on: 914938
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).
Attachment #802614 - Flags: review?(mwu)
(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.
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.
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
Flags: needinfo?(mwu)
Blocks: 911560
Blocks: 919590
Blocks: 919596
(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.
Flags: needinfo?(mwu)
Thanks for the comment. I am going to implement the patch by using the software color conversion.
Attachment #802614 - Attachment is obsolete: true
Attachment #802607 - Attachment is obsolete: true
Attachment #802607 - Flags: review?(mwu)
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.
Attachment #809228 - Flags: review?(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.
Attachment #809228 - Flags: review?(mwu) → review+
(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.
(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;
     }
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.
Okey, I am going to split the change to own commit.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Oh, I need to uplift the change to "b2g-4.3_r2.1".
Yeah - sorry I missed that. Please reset the master branch to its previous state after uplifting.
(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.
reset of mater is completed. mwu, thanks for your help.
(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.
Flags: needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g)
Inder, does caf provide the support for nexus4 build? And can you give me a link of corresponding color conversion code?
Flags: needinfo?(ikumar)
And I am not the person to make decision to use AOSP for nexus4 moz build.
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.
I add POVB on the whiteborad.
Whiteboard: POVB
(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
Flags: needinfo?(ikumar)
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
Whiteboard: POVB → POVB, QARegressExclude
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
Flags: needinfo?(ikumar)
Sotaro -- we didn't have plans to add the system property. Do we need this for proper shutter sound functioning?
Flags: needinfo?(ikumar) → needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g)
You need to log in before you can comment on or make changes to this bug.