1219142 - Use MediaEventSource to publish events when data arrives in the MediaResource

Status: RESOLVED FIXED

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

Description

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

The reader can register with the event to receive notifications without delegation from MDSM. This will also remove MediaDecoderReader::DispatchNotifyDataArrived to simplify the interface.

Comment 1

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

Attachment: MozReview Request: Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.

Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.

Comment 2

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

Attachment: MozReview Request: Bug 1219142. Part 2 - remove unused code. r=jya.

Bug 1219142. Part 2 - remove unused code. r=jya.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681769 [details]
MozReview Request: Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.

https://reviewboard.mozilla.org/r/23889/#review21337

::: dom/media/AbstractMediaDecoder.h:81
(Diff revision 1)
> +  virtual MediaEventSource<uint32_t, int64_t, bool>* DataArrivedEvent()

I'd prefer that no such information is provided, as it can only lead to invalid reads being performed on the MediaResource.

So instead, don't have any arguments at all.

Since bug 1218157; the arguments are unused.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681770 [details]
MozReview Request: Bug 1219142. Part 2 - remove unused code. r=jya.

https://reviewboard.mozilla.org/r/23891/#review21339

Comment 5

[Jean-Yves Avenard [:jya]]

NotifyDataArrivedInternal , if still used should also no longer been provided argument.

The receiver should only been using the call to detect that the buffered range is to be refreshed. not attempt to do anything else

Comment 6

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

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> NotifyDataArrivedInternal , if still used should also no longer been
> provided argument.

https://hg.mozilla.org/mozilla-central/file/96377bdbcdf3/dom/media/webm/WebMReader.cpp#l805
I guess it should be |mBufferedState->NotifyDataArrived(bytes->Elements(), bytes->Length(), range.mStart)| so |aLength| and |aOffset| won't be used at all.

Comment 7

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

https://reviewboard.mozilla.org/r/23889/#review21337

> I'd prefer that no such information is provided, as it can only lead to invalid reads being performed on the MediaResource.
> 
> So instead, don't have any arguments at all.
> 
> Since bug 1218157; the arguments are unused.

Do you mean |virtual MediaEventSource<void>* DataArrivedEvent()| because we don't need |aThrottleUpdates| either? However, MediaDecoderReader::ThrottledNotifyDataArrived() still takes an interval which comes from |media::Interval<int64_t>(aOffset, aOffset + aLength)| in MediaDecoderReader::OnDataArrived().

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #6)
> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > NotifyDataArrivedInternal , if still used should also no longer been
> > provided argument.
> 
> https://hg.mozilla.org/mozilla-central/file/96377bdbcdf3/dom/media/webm/
> WebMReader.cpp#l805
> I guess it should be |mBufferedState->NotifyDataArrived(bytes->Elements(),
> bytes->Length(), range.mStart)| so |aLength| and |aOffset| won't be used at
> all.

But those aren't the arguments provided to WebMReader::NotifyDataArrived

Instead it retrieves the cached ranges and pass that information instead.

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #7)
> https://reviewboard.mozilla.org/r/23889/#review21337
> 
> > I'd prefer that no such information is provided, as it can only lead to invalid reads being performed on the MediaResource.
> > 
> > So instead, don't have any arguments at all.
> > 
> > Since bug 1218157; the arguments are unused.
> 
> Do you mean |virtual MediaEventSource<void>* DataArrivedEvent()| because we
> don't need |aThrottleUpdates| either? However,
> MediaDecoderReader::ThrottledNotifyDataArrived() still takes an interval
> which comes from |media::Interval<int64_t>(aOffset, aOffset + aLength)| in
> MediaDecoderReader::OnDataArrived().

Same thing.. We no longer require this. We could remove the throttling too...
We may want to reschedule a task when MediaFormatReader::NotifyDataArrivedInternal is called; so that when we get a storm of NotifyDataArrived dispatch (it's called for every 32kB downloaded) we don't end up recalculating unnecessarily, and instead it's calculated once only. Actually, upon checking it's already done that way.

Comment 10

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

https://reviewboard.mozilla.org/r/23889/#review22535

Comment 11

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/23889/#review22535

is there something new I should look at?

Comment 12

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

no, just a rebase for we remove all arguments (throttling and others).

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #12)
> no, just a rebase for we remove all arguments (throttling and others).

but I still see the throttle related arguments in that patch.

Comment 14

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

Can you paste the link? I see none in this link: https://reviewboard.mozilla.org/r/23889/diff/2#index_header.

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #14)
> Can you paste the link? I see none in this link:
> https://reviewboard.mozilla.org/r/23889/diff/2#index_header.

"The page you were looking for does not exist."

I see: https://reviewboard.mozilla.org/r/23889/diff/1#2
like dom/media/MediaDecoder.cpp
" mDataArrivedEvent.Notify(aLength, aOffset, aThrottleUpdates);"

etc.

Comment 16

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

How about this one? https://reviewboard.mozilla.org/r/23889/diff/2/

Comment 17

[Jean-Yves Avenard [:jya]]

still no go:
"The page you were looking for does not exist.

If you're pretty sure this page does exist, try logging in and trying again."

but I'm logged in

Comment 18

[Jean-Yves Avenard [:jya]]

This is the only link to a repository I can find:
https://reviewboard-hg.mozilla.org/gecko/rev/ba0fb2ebab3d

and that's the code from 2 weeks ago

Comment 19

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

Let me upload the patch again.

Comment 20

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

Comment on attachment 8681769 [details]
MozReview Request: Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.

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

Comment 21

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

Comment on attachment 8681770 [details]
MozReview Request: Bug 1219142. Part 2 - remove unused code. r=jya.

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

Comment 22

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

Does it work? https://reviewboard.mozilla.org/r/23887

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681769 [details]
MozReview Request: Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.

https://reviewboard.mozilla.org/r/23889/#review22831

sorry it took so long. at no time did i see a new revision (until now that is)

Comment 24

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

Thanks for the review!

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74726f6aa4e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/28d11278ff97

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74726f6aa4e9
https://hg.mozilla.org/mozilla-central/rev/28d11278ff97