Bruce Sun [:brsun] Assignee
	Description • 11 years ago

If BufferInfo::mStatus of one BufferInfo in OMXCode::mPortBuffers[kPortIndexOutput] is OWNED_BY_US, that means this BufferInfo is either an unused empty buffer hold by OMXCodec, or a ready-to-be-retrieved buffer filled with decoded data hold by OMXCodec.

In OMXCodec::fillOutputBuffers(), all BufferInfos with OWNED_BY_US status are sent to the underlying component for being filled with decoded data in the future.

However, OMXCodec should check whether BufferInfo contains decoded data already or not before sending it to the underlying component.

ref: https://bugzilla.mozilla.org/show_bug.cgi?id=990908#c19
ref: https://bug990908.bugzilla.mozilla.org/attachment.cgi?id=8407471

	Bruce Sun [:brsun] Assignee
	Comment 1 • 11 years ago

Check mFilledBuffers before filling output buffers.

	Eric Chou [:ericchou] [:echou]
	Comment 2 • 11 years ago

PR merged.

https://github.com/mozilla-b2g/platform_frameworks_av/commit/6c67114dfa109f31d37e880b9c009f2965a22261

	Bruce Sun [:brsun] Assignee
	Comment 3 • 11 years ago

Check mFilledBuffers before filling output buffers.

The same as attachment 8413579 [details], but this one is for JB (mozilla-b2g:b2g-4.4.2_r1).

	Bruce Sun [:brsun] Assignee
	Comment 4 • 11 years ago

This issue fix the root cause of the crash in bug 990908.

	Bruce Sun [:brsun] Assignee
	Comment 5 • 11 years ago

Check mFilledBuffers before filling output buffers.

The same as attachment 8413579 [details], but this one is for JB (mozilla-b2g:b2g-4.4.2_r1).

p.s. attachment 8414190 [details] points to the same pull request of attachment 8413579 [details], which is still KK, not JB.

	Bruce Sun [:brsun] Assignee
	Comment 6 • 11 years ago

(In reply to Bruce Sun [:brsun] from comment #5)
> The same as attachment 8413579 [details], but this one is for JB
> (mozilla-b2g:b2g-4.4.2_r1).
   ^^^^^^^^^^^^^^^^^^^^^^^^
typo, it should be mozilla-b2g:b2g-4.3_r2.1

	Eric Chou [:ericchou] [:echou]
	Comment 7 • 11 years ago

PR merged.

https://github.com/mozilla-b2g/platform_frameworks_av/commit/fa40716703edc8664365b46d206c11635d2b7a90

	Sotaro Ikeda [:sotaro]
	Comment 8 • 10 years ago

This bug is flagged as "b2g-2.0+". But this fix is not applied to caf OMXCodec. I checked caf JB's OMXCodec and caf KK's OMXCodec and I do not see the fix.

	Sotaro Ikeda [:sotaro]
	Comment 9 • 10 years ago

brsun, do you notice about Comment 8?

	Bruce Sun [:brsun] Assignee
	Comment 10 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> brsun, do you notice about Comment 8?

Yes. It seems this patch has not been integrated in CAF yet.

mvines: do you have any interests to integrate this patch into JB's OMXCodec or KK's OMXCodec?

	Michael Vines [:m1] [:evilmachines]
	Comment 11 • 10 years ago

Why do we need it for KK?  That's unclear from comment 0.  I won't pick this up for JB.

	Bruce Sun [:brsun] Assignee
	Comment 12 • 10 years ago

(In reply to Michael Vines [:m1] [:evilmachines] from comment #11)
> Why do we need it for KK?  That's unclear from comment 0.  I won't pick this
> up for JB.

There is an potential crash issue in OMXCodec. Although gecko did some workaround on bug 990908, the root cause hasn't been fixed.

In OMXCodec::read(), after one buffer has been retrieved by using |mFilledBuffers.begin()|, there is one hard checking on the corresponding BufferInfo: |CHECK_EQ((int)info->mStatus, (int)OWNED_BY_US)|. In some circumstances OMXCodec fails to pass that checking and then crashes.

The root cause of this crash is that OMXCodec::fillOutputBuffers() only checks |if (info->mStatus == OWNED_BY_US)| before sending an individual buffer to the underlying component. After being sent by OMXCodec::fillOutputBuffers(), |info->mStatus| will be changed into OWNED_BY_COMPONENT. Since all buffers indexed by |mFilledBuffers| should be OWNED_BY_US by OMXCodec's design, if OMXCodec::fillOutputBuffers() sends any buffers which are already indexed by mFilledBuffers, OMXCodec crashes at that checking in OMXCodec::read().

In order to avoid such unexpected crashes, an extra checking on mFilledBuffers would be needed in OMXCodec::fillOutputBuffers().

	Bruce Sun [:brsun] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8413579 [details]
Check mFilledBuffers before filling output buffers.

Add feedback? since this patch hasn't been feedback by our partners yet.

ref: https://bugzilla.mozilla.org/show_bug.cgi?id=990908#c24

	Ming
	Comment 14 • 10 years ago

Almost forgot this. 
Thanks ,i'll handle pick up this patch on sprd side.

	James Zhang (Spreadtrum)
	Comment 15 • 10 years ago

Landed on my side.

Bug #329382 BufferInfo::mStatus in OMXCodec::mFilledBuffers are not handled well in OMXCodec::fillOutputBuffers().

[bug number  ] 329382
[root cause  ] refs to moz bug:https://bugzilla.mozilla.org/show_bug.cgi?id=1002338
[changes     ] OMXCodec.cpp
[side effects] none
[self test   ] yes
[whether AOB ] yes
[reviewers   ]
Change-Id: I45fed0da5b1240e6ed38d46f5be7feb3720bed74

	Bruce Sun [:brsun] Assignee
	Comment 16 • 10 years ago

mvines: Hi, it would be good to have your comments or feedback as well (refer to attachment 8413579 [details] and comment 12.)