Closed Bug 1124542 Opened 5 years ago Closed 5 years ago

WebrtcGmpVideoDecoder shouldn't crash when GMP completion callbacks are received

Categories

(Core :: WebRTC: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 - fixed
firefox38 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file)

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.
Attached patch patch v0Splinter Review
Attachment #8552865 - Flags: review?(rjesup)
Blocks: 1121258
Attachment #8552865 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/703b286c822a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
I believe we should uplift this to 37 - see bug 1140520
Blocks: 1140520
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?
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup)
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+
This isn't critical for release and doesn't need to be tracked.
You need to log in before you can comment on or make changes to this bug.