1221009 - Separate the interface of MediaResourceCallback from MediaDecoder

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[JW Wang [:jwwang] [:jw_wang]]

We will have a class inheriting MediaResourceCallback to forward notifications from MediaResource to MediaDecoder which no longer inherits MediaResourceCallback.

There are several benefits in doing this:
1. We will be able to create MediaDecoder after MediaResource is instantiated and remove the need for MediaDecoder::SetResource() by passing MediaResource to the constructor. This allow us to make mResource const and simplify the logic.

2. MediaDecoder can be disconnected from the object to stop receiving notifications from MediaResource. We won't have to check if MediaDecoder is in a valid state (shutdown or not) to process the notifications.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1221009. Part 1 - add a class to forward notifications from MediaResource to MediaDecoder. r=roc.

Bug 1221009. Part 1 - add a class to forward notifications from MediaResource to MediaDecoder. r=roc.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1221009. Part 2 - remove unused code. r=roc.

Bug 1221009. Part 2 - remove unused code. r=roc.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1221009. Part 3 - add assertions to functions that shouldn't be called after shutdown. r=roc.

Bug 1221009. Part 3 - add assertions to functions that shouldn't be called after shutdown. r=roc.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686528 [details]
MozReview Request: Bug 1221009. Part 1 - add a class to forward notifications from MediaResource to MediaDecoder. r=roc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24995/diff/1-2/

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For some reason I can't review those patches. Others do work. Maybe you need to repost them?

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686528 [details]
MozReview Request: Bug 1221009. Part 1 - add a class to forward notifications from MediaResource to MediaDecoder. r=roc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24995/diff/1-2/

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686529 [details]
MozReview Request: Bug 1221009. Part 2 - remove unused code. r=roc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24997/diff/1-2/

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8686530 [details]
MozReview Request: Bug 1221009. Part 3 - add assertions to functions that shouldn't be called after shutdown. r=roc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24999/diff/1-2/

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

reposted. Does it work?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, thanks.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8686528 [details]
MozReview Request: Bug 1221009. Part 1 - add a class to forward notifications from MediaResource to MediaDecoder. r=roc.

https://reviewboard.mozilla.org/r/24995/#review22725

This is only a very small improvement IMHO, but since you've done it, I guess we might as well take it.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8686529 [details]
MozReview Request: Bug 1221009. Part 2 - remove unused code. r=roc.

https://reviewboard.mozilla.org/r/24997/#review22727

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8686530 [details]
MozReview Request: Bug 1221009. Part 3 - add assertions to functions that shouldn't be called after shutdown. r=roc.

https://reviewboard.mozilla.org/r/24999/#review22729

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> This is only a very small improvement IMHO, but since you've done it, I
> guess we might as well take it.

I mean, it probably wasn't worth your time to do this.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

This is a step forward to remove bi-directional dependency between MediaDecoder and MDSM/MediaResource. For now, MediaDecoder functions could be called after Shutdown() and we have to check |mShuttingDown| from time to time to know if this MediaDecoder is in a valid state to handle the request.

I would like to apply the observer pattern so MediaDecoder can convenient disconnect from the event publishers (MDSM or MediaResource) during shutdown to stop receiving notifications. It also helps improve the logic of MediaDecoder in a more readable way.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e757bbcf37

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/926b9011f5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a20ad2282ed1
https://hg.mozilla.org/integration/mozilla-inbound/rev/057d396895c6

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/926b9011f5c2
https://hg.mozilla.org/mozilla-central/rev/a20ad2282ed1
https://hg.mozilla.org/mozilla-central/rev/057d396895c6