1123054 - Mac VDA decoder will assert when decoding error occurs.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

Silly mistake in AppleVDADecoder.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Only check reference count if no decoding error occurred

In case of error, retain count will only be 1.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Only check reference count if no decoding error occurred

Actually, don't need it at all. And refcount could be 1 if the frame was immediately output

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8550887 [details] [diff] [review]
Only check reference count if no decoding error occurred

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

Right, the reference count must be >= 1 here, because the AutoCFRelease local holds the reference returned by CFDictionaryCreate, but the callback could have already returned, and then VDADecoderDecode would have released its internal reference.

I agree the assert isn't necessary.

Comment 4

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5ae67f3b1e

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a5ae67f3b1e

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8550887 [details] [diff] [review]
Only check reference count if no decoding error occurred

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes playing video on Mac.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Minimal. At worst it will crash somewhere else.
[String/UUID change made/needed]: None.

We're only targeting Windows in beta 36, but this affects normal video playback on Mac, so I think it's worth uplifting to both branches for consistency. It will be harder to trigger the bug it fixes without MSE, but there could still be some user benefit.

We're targeting MacOS X in 37, so it should definitely go into Aurora.

Comment 7

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a48f8c55a98c

Comment 8

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f10730d7411a