Closed
Bug 1002338
Opened 11 years ago
Closed 11 years ago
BufferInfo::mStatus in OMXCodec::mFilledBuffers are not handled well in OMXCodec::fillOutputBuffers().
Categories
(Core :: Audio/Video, defect)
Tracking
()
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: brsun, Assigned: brsun)
References
Details
Attachments
(2 files, 1 obsolete file)
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
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → brsun
Assignee | ||
Comment 1•11 years ago
|
||
Check mFilledBuffers before filling output buffers.
Attachment #8413579 -
Flags: review?(sotaro.ikeda.g)
Updated•11 years ago
|
Attachment #8413579 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 2•11 years ago
|
||
PR merged. https://github.com/mozilla-b2g/platform_frameworks_av/commit/6c67114dfa109f31d37e880b9c009f2965a22261
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 3•11 years ago
|
||
Check mFilledBuffers before filling output buffers. The same as attachment 8413579 [details] [review], but this one is for JB (mozilla-b2g:b2g-4.4.2_r1).
Assignee | ||
Comment 4•11 years ago
|
||
This issue fix the root cause of the crash in bug 990908.
blocking-b2g: --- → 2.0?
Assignee | ||
Comment 5•11 years ago
|
||
Check mFilledBuffers before filling output buffers. The same as attachment 8413579 [details] [review], but this one is for JB (mozilla-b2g:b2g-4.4.2_r1). p.s. attachment 8414190 [details] [review] points to the same pull request of attachment 8413579 [details] [review], which is still KK, not JB.
Attachment #8414190 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8414201 -
Attachment description: Check mFilledBuffers before filling output buffers. (mozilla-b2g:b2g-4.4.2_r1) → Check mFilledBuffers before filling output buffers. (mozilla-b2g:b2g-4.3_r2.1)
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Bruce Sun [:brsun] from comment #5) > The same as attachment 8413579 [details] [review], but this one is for JB > (mozilla-b2g:b2g-4.4.2_r1). ^^^^^^^^^^^^^^^^^^^^^^^^ typo, it should be mozilla-b2g:b2g-4.3_r2.1
Comment 7•11 years ago
|
||
PR merged. https://github.com/mozilla-b2g/platform_frameworks_av/commit/fa40716703edc8664365b46d206c11635d2b7a90
Updated•11 years ago
|
status-b2g-v2.0:
--- → fixed
Target Milestone: --- → 2.0 S1 (9may)
Updated•11 years ago
|
blocking-b2g: 2.0? → 2.0+
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.
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?
Flags: needinfo?(brsun) → needinfo?(mvines)
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.
Flags: needinfo?(mvines)
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().
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8413579 [details] [review] 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
Attachment #8413579 -
Flags: feedback?(mvines)
Attachment #8413579 -
Flags: feedback?(james.zhang)
Updated•10 years ago
|
Flags: needinfo?(ming.li)
Comment 14•10 years ago
|
||
Almost forgot this. Thanks ,i'll handle pick up this patch on sprd side.
Flags: needinfo?(ming.li)
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
Updated•10 years ago
|
Attachment #8413579 -
Flags: feedback?(james.zhang) → feedback+
Updated•10 years ago
|
Attachment #8413579 -
Flags: feedback?(mvines)
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] [review] and comment 12.)
Flags: needinfo?(mvines)
Updated•10 years ago
|
Flags: needinfo?(mvines)
You need to log in
before you can comment on or make changes to this bug.
Description
•