1195158 - Remove MediaDecoder::QueueMetadata

Status: RESOLVED FIXED

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

Description

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

Because it doesn't run on the main thread.

Comment 1

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

Bug 1194112 allows us to send move-only types to another thread. We will be able to send TimedMetadata to MediaDecoder using MediaEventSource.

Comment 2

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce77a4423e8

Comment 3

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

Attachment: MozReview Request: Bug 1195158. Part 1 - Have MediaMetadataManager listen to an event source to receive TimedMetadata events. OggReader will send TimedMetadata events through an event source.

Bug 1195158. Part 1 - Have MediaMetadataManager listen to an event source to receive TimedMetadata events. OggReader will send TimedMetadata events through an event source. This will break OggReader's dependency on AbstractMediaDecoder::QueueMetadata which then can be removed for it is against our goal to run all MediaDecoder's methods on the main thread.

Comment 4

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

Attachment: MozReview Request: Bug 1195158. Part 2 - Have OggReader send TimedMetadata events through a event source instead of direct calls to AbstractMediaDecoder::QueueMetadata. r=cpearce.

Bug 1195158. Part 2 - Have OggReader send TimedMetadata events through a event source instead of direct calls to AbstractMediaDecoder::QueueMetadata.

Comment 5

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

Attachment: MozReview Request: Bug 1195158. Part 3 - connect listeners. r=cpearce.

Bug 1195158. Part 3 - connect listeners.
a. MediaMetadataManager is connected to MediaDecoderReader::mTimedMetadataEvent to receive TimedMetadata events.
b. OggReader publish TimedMetadata events through MediaDecoderReader::mTimedMetadataEvent.
c. MDSM calls MediaMetadataManager::DispatchMetadataIfNeeded to publish metadata if playback positoin reaches the publish time.
d. MediaDecoder is connected to MediaMetadataManager::mTimedMetadataEvent to receive TimedMetadata events.
e. MediaDecoder updates its metadata when TimedMetadata events are received.

Comment 6

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

Attachment: MozReview Request: Bug 1195158. Part 4 - remove unused code. r=cpearce.

Bug 1195158. Part 4 - remove unused code.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8648557 [details]
MozReview Request: Bug 1195158. Part 1 - Have MediaMetadataManager listen to an event source to receive TimedMetadata events. OggReader will send TimedMetadata events through an event source.

https://reviewboard.mozilla.org/r/16239/#review14581

Ship It!

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8648558 [details]
MozReview Request: Bug 1195158. Part 2 - Have OggReader send TimedMetadata events through a event source instead of direct calls to AbstractMediaDecoder::QueueMetadata. r=cpearce.

https://reviewboard.mozilla.org/r/16241/#review14583

Ship It!

::: dom/media/ogg/OggReader.cpp:820
(Diff revision 1)
> +                      nsAutoPtr<MetadataTags>(tags),

|tags| is already an nsAutoPtr, defined on line 734, so you shouldn't need to wrap it in another MediadataTags constructor here.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8648559 [details]
MozReview Request: Bug 1195158. Part 3 - connect listeners. r=cpearce.

https://reviewboard.mozilla.org/r/16243/#review14589

Ship It!

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8648560 [details]
MozReview Request: Bug 1195158. Part 4 - remove unused code. r=cpearce.

https://reviewboard.mozilla.org/r/16245/#review14591

Ship It!

Comment 11

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

(In reply to Chris Pearce (:cpearce) from comment #8)
> ::: dom/media/ogg/OggReader.cpp:820
> (Diff revision 1)
> > +                      nsAutoPtr<MetadataTags>(tags),
> 
> |tags| is already an nsAutoPtr, defined on line 734, so you shouldn't need
> to wrap it in another MediadataTags constructor here.

Ah, right! Thanks for the review.
Will change |nsAutoPtr<MetadataTags>(tags)| to |Move(tags)|.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60a3b1862f71
https://hg.mozilla.org/integration/mozilla-inbound/rev/157e41e32906
https://hg.mozilla.org/integration/mozilla-inbound/rev/6226b99f19bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7610baf4a3ae

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/adc449224816 for

W(3) failure, at least on Linux opts:
PROCESS-CRASH | /media-source/mediasource-multiple-attach.html | application crashed [@ mozilla::MediaEventListener::Disconnect()]

W(5) failure, at least on Linux and OSX debug:

PROCESS | 1865 | Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsRefPtr with operator->().), at ../../dist/include/mozilla/nsRefPtr.h:286
PROCESS-CRASH | /media-source/mediasource-multiple-attach.html | application crashed [@ nsRefPtr<mozilla::RevocableToken>::operator->() const]

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

From Nexus 5-L(Be):
../../dist/include/MediaDecoder.h:986:25: error: 'TimedMetadata' has not been declared

Comment 15

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

Comment on attachment 8648557 [details]
MozReview Request: Bug 1195158. Part 1 - Have MediaMetadataManager listen to an event source to receive TimedMetadata events. OggReader will send TimedMetadata events through an event source.

Bug 1195158. Part 1 - Have MediaMetadataManager listen to an event source to receive TimedMetadata events. OggReader will send TimedMetadata events through an event source. This will break OggReader's dependency on AbstractMediaDecoder::QueueMetadata which then can be removed for it is against our goal to run all MediaDecoder's methods on the main thread. r=cpearce.

Comment 16

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

Comment on attachment 8648558 [details]
MozReview Request: Bug 1195158. Part 2 - Have OggReader send TimedMetadata events through a event source instead of direct calls to AbstractMediaDecoder::QueueMetadata. r=cpearce.

Bug 1195158. Part 2 - Have OggReader send TimedMetadata events through a event source instead of direct calls to AbstractMediaDecoder::QueueMetadata. r=cpearce.

Comment 17

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

Comment on attachment 8648559 [details]
MozReview Request: Bug 1195158. Part 3 - connect listeners. r=cpearce.

Bug 1195158. Part 3 - connect listeners. r=cpearce.
a. MediaMetadataManager is connected to MediaDecoderReader::mTimedMetadataEvent to receive TimedMetadata events.
b. OggReader publish TimedMetadata events through MediaDecoderReader::mTimedMetadataEvent.
c. MDSM calls MediaMetadataManager::DispatchMetadataIfNeeded to publish metadata if playback positoin reaches the publish time.
d. MediaDecoder is connected to MediaMetadataManager::mTimedMetadataEvent to receive TimedMetadata events.
e. MediaDecoder updates its metadata when TimedMetadata events are received.

Comment 18

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

Comment on attachment 8648560 [details]
MozReview Request: Bug 1195158. Part 4 - remove unused code. r=cpearce.

Bug 1195158. Part 4 - remove unused code. r=cpearce.

Comment 19

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

Attachment: MozReview Request: Bug 1195158. Part 5 - 1. Fix insufficient includes and sort out include order. 2. Only disconnect |mTimedMetadataListener| when the state machine is created.

Bug 1195158. Part 5 - 1. Fix insufficient includes and sort out include order. 2. Only disconnect |mTimedMetadataListener| when the state machine is created.

Comment 20

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

Patch part 5.

Fix insufficient includes in comment 14.
Fix the crash in comment 13. MediaDecoder can be shut down without creating the state machine. The listener should only be disconnected when the state machine has been created.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8652098 [details]
MozReview Request: Bug 1195158. Part 5 - 1. Fix insufficient includes and sort out include order. 2. Only disconnect |mTimedMetadataListener| when the state machine is created.

https://reviewboard.mozilla.org/r/17073/#review16449

Comment 22

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

Thanks!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a84b7facd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/642778802ad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/778e3577ee7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/36369d9491bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/365a1e874a93

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9a84b7facd4
https://hg.mozilla.org/mozilla-central/rev/642778802ad8
https://hg.mozilla.org/mozilla-central/rev/778e3577ee7d
https://hg.mozilla.org/mozilla-central/rev/36369d9491bc
https://hg.mozilla.org/mozilla-central/rev/365a1e874a93