1125047 - doesn't response to decoder error in GMP module

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[karina]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/600.2.5 (KHTML, like Gecko) Version/8.0.2 Safari/600.2.5

Comment 1

[karina]

issue description:
Firefox squared client calls with iOS squared client.FF client's video suffers from frequent and long freezes. 
root cause:
decoder failed status doesn't be got in WebrtcGmpVideoCodec.cpp. so there is no IDR request.

Comment 2

[karina]

Attachment: decoder_patch.txt

Comment 3

[karina]

it needs to work with openh264 commit  6afdf36

Comment 4

[Ethan Hugg [:ehugg]]

This appears to match up with this pull req - https://github.com/cisco/openh264/pull/1751

I assume that this could be landed with the current plugin without breaking the current functionality?

Comment 5

[karina]

yes, it will not  break the current functionality.

Comment 6

[Ethan Hugg [:ehugg]]

OK, it worked for me with the current plugin.  I'll request a review from Randell.  Here's a try of some desktop builds to try it out:

https://tbpl.mozilla.org/?tree=Try&rev=79a6a7d8b2f0

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8553601 [details] [diff] [review]
decoder_patch.txt

Review of attachment 8553601 [details] [diff] [review]:
-----------------------------------------------------------------

r+, since Error() and Decode_g() are both on the GMP thread

Comment 8

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d696936757e

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d696936757e

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

[Tracking Requested - why for this release]:
This would enable pushing OpenH264 1.4 for Fx37. 1.4 will have a fix for interop issues with Chromebook's H.264 support.  (Note that OpenH264 releases separately from Firefox, but this is an enabling (and very simple/low-risk) patch).

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Randell, are you asking for this to be uplifted to beta? 
If so, can you ask for approval for uplift on the patch?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

I'm going to request it as soon as ehugg verifies these with OpenH264 1.4; he's working on that now

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8553601 [details] [diff] [review]
decoder_patch.txt

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Inability to ship OpenH264 1.4 for 37

[Describe test coverage new/current, TreeHerder]: On 38/39 for a fair while; manually tested with OpenH264 1.4 (and just retested with 1.3 and current 1.4 build)

[Risks and why]: Virtually no risk.  Trivial patch to ensure error returns are passed up the chain so we can recover properly from decoding errors.

[String/UUID change made/needed]: none

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8553601 [details] [diff] [review]
decoder_patch.txt

This fix has already been verified and improves our options for interop. Beta+

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

This doesn't need to be tracked.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5598a289b442

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf49b6b96324
Tested Windows and Linux debug & opt builds from the try manually with the current 1.3 plugin; ehugg tested with 1.3 and 1.4-candidate, both existing and new profiles.  Tested repeated calls, and also manually crashing the plugin.