Closed Bug 1124542 Opened 5 years ago Closed 5 years ago
Gmp Video Decoder shouldn't crash when GMP completion callbacks are received
For the changes required to make the GMP PDM in bug 1121258 work, commit 597502f in my OpenH264 GMP pull request (https://github.com/cisco/openh264/pull/1746) adds calls to InputDataExhausted, DrainComplete, and FlushComplete. WebrtcGmpVideoDecoder implements these callbacks with a MOZ_CRASH(), so the updated OpenH264 GMP would cause it to crash when these callbacks are called.
Attachment #8552865 - Flags: review?(rjesup) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
We would only need this in 37 if we updated the OpenH264 of FF37 to 1.4 which we will not be doing. 1.4 will be 38+ or later. Sorry for the confusion.
No longer blocks: 1140520
[Tracking Requested - why for this release]: Needed to enable using OpenH264 1.4 with Firefox 37. We may want the option to push 1.4 to Fx37 (after 37 goes to release) to enable H.264 interop with Chromebooks in WebRTC.
The last 37 Beta goes to build this Thu, Mar 19. If we need to enable OpenH264 with Firefox 37, we should take this patch in Beta 7. If not, we can defer the change to 38 where it has already landed. Randell - This looks like an isolated change but I don't know the potential impact of taking out these crash statements. 1. Do we need this in 37? 2. What risk is there with making this change?
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #7) > Randell - This looks like an isolated change but I don't know the potential > impact of taking out these crash statements. > 1. Do we need this in 37? If we think we may want to ship OpenH264 for 37 at any point, we need this (and the other patch) to be in. > 2. What risk is there with making this change? Extremely low. The MOZ_CRASHes were just paranoia (and have since been removed), and the lack of providing the error codes is basically riskless. Both have been in 38 for quite some time.
Comment on attachment 8552865 [details] [diff] [review] patch v0 Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: Blocks Deploying OpenH264 1.4 for 37 [Describe test coverage new/current, TreeHerder]: On 38/39 for a long time Manual testing with OpenH264, and also tested by EME code that uses completion callbacks. [Risks and why]: No risk worth speaking of; these were paranoia MOZ_CRASHes [String/UUID change made/needed]: none
Attachment #8552865 - Flags: approval-mozilla-beta?
Comment on attachment 8552865 [details] [diff] [review] patch v0 Low risk fix that has been verified and improves our options for interop. Beta+
Attachment #8552865 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.