Closed Bug 1123054 Opened 5 years ago Closed 5 years ago

Mac VDA decoder will assert when decoding error occurs.

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

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

People

(Reporter: jya, Assigned: jya)

Details

Attachments

(1 file, 1 obsolete file)

Silly mistake in AppleVDADecoder.
In case of error, retain count will only be 1.
Attachment #8550881 - Flags: review?(giles)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Actually, don't need it at all. And refcount could be 1 if the frame was immediately output
Attachment #8550887 - Flags: review?(giles)
Attachment #8550881 - Attachment is obsolete: true
Attachment #8550881 - Flags: review?(giles)
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.
Attachment #8550887 - Flags: review?(giles) → review+
https://hg.mozilla.org/mozilla-central/rev/7a5ae67f3b1e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8550887 - Flags: approval-mozilla-beta?
Attachment #8550887 - Flags: approval-mozilla-aurora?
Attachment #8550887 - Flags: approval-mozilla-beta?
Attachment #8550887 - Flags: approval-mozilla-beta+
Attachment #8550887 - Flags: approval-mozilla-aurora?
Attachment #8550887 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.