1378316 - Remove AbstractMediaDecoder::DataArrivedEvent()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Comment 1

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

Attachment: Bug 1378316. P1 - store MediaDecoderReader in MediaDecoder.

So we are able to dispatch NotifyDataArrived() to MediaDecoderReader in P2.

Review commit: https://reviewboard.mozilla.org/r/154824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154824/

Comment 2

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

Attachment: Bug 1378316. P2 - dispatch MediaDecoderReader::NotifyDataArrived explicitly instead of using MediaEventSource.

Review commit: https://reviewboard.mozilla.org/r/154826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154826/

Comment 3

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

Attachment: Bug 1378316. P3 - remove AbstractMediaDecoder::DataArrivedEvent() and related code.

Review commit: https://reviewboard.mozilla.org/r/154828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154828/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8883861 [details]
Bug 1378316. P1 - store MediaDecoderReader in MediaDecoder.

https://reviewboard.mozilla.org/r/154824/#review160066

r+ with suggestion:

::: dom/media/fmp4/MP4Decoder.cpp:292
(Diff revision 1)
>  }
>  
>  void
>  MP4Decoder::GetMozDebugReaderData(nsACString& aString)
>  {
> -  if (mReader) {
> +  auto reader = static_cast<MediaFormatReader*>(mReader.get());

Downcasting is the one ugly thing about this changeset!

But fixing it (e.g., virtualizing GetMozDebugReaderData in MediaDecoderReader and MediaFormatReader) would probably be more work than is worth for a debugging function, so don't worry about it unless it's easy enough.

Maybe just add a comment with each downcast, to note that you "know it's a MediaFormatReader because ..."

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8883862 [details]
Bug 1378316. P2 - dispatch MediaDecoderReader::NotifyDataArrived explicitly instead of using MediaEventSource.

https://reviewboard.mozilla.org/r/154826/#review160076

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8883863 [details]
Bug 1378316. P3 - remove AbstractMediaDecoder::DataArrivedEvent() and related code.

https://reviewboard.mozilla.org/r/154828/#review160078

Comment 7

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

Comment on attachment 8883861 [details]
Bug 1378316. P1 - store MediaDecoderReader in MediaDecoder.

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

Comment 8

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

Comment on attachment 8883862 [details]
Bug 1378316. P2 - dispatch MediaDecoderReader::NotifyDataArrived explicitly instead of using MediaEventSource.

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

Comment 9

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

Comment on attachment 8883863 [details]
Bug 1378316. P3 - remove AbstractMediaDecoder::DataArrivedEvent() and related code.

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

Comment 10

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

Thanks for the review!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a357ac5ab0f8
P1 - store MediaDecoderReader in MediaDecoder. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4987a415f22d
P2 - dispatch MediaDecoderReader::NotifyDataArrived explicitly instead of using MediaEventSource. r=gerald
https://hg.mozilla.org/integration/autoland/rev/78ff4c023b6a
P3 - remove AbstractMediaDecoder::DataArrivedEvent() and related code. r=gerald

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a357ac5ab0f8
https://hg.mozilla.org/mozilla-central/rev/4987a415f22d
https://hg.mozilla.org/mozilla-central/rev/78ff4c023b6a