Closed Bug 1219142 Opened 6 years ago Closed 6 years ago

Use MediaEventSource to publish events when data arrives in the MediaResource

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

The reader can register with the event to receive notifications without delegation from MDSM. This will also remove MediaDecoderReader::DispatchNotifyDataArrived to simplify the interface.
Bug 1219142. Part 1 - add AbstractMediaDecoder::DataArrivedEvent() to publish events. r=jya.
Attachment #8681769 - Flags: review?(jyavenard)
Bug 1219142. Part 2 - remove unused code. r=jya.
Attachment #8681770 - Flags: review?(jyavenard)
Attachment #8681769 - Flags: review?(jyavenard)
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 on attachment 8681770 [details]
MozReview Request: Bug 1219142. Part 2 - remove unused code. r=jya.

https://reviewboard.mozilla.org/r/23891/#review21339
Attachment #8681770 - Flags: review?(jyavenard) → review+
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
(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.
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().
(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.
(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.
Depends on: 1220551
Depends on: 1220558
Priority: -- → P2
Depends on: 1223599
Attachment #8681769 - Flags: review?(jyavenard)
https://reviewboard.mozilla.org/r/23889/#review22535

is there something new I should look at?
no, just a rebase for we remove all arguments (throttling and others).
(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.
Can you paste the link? I see none in this link: https://reviewboard.mozilla.org/r/23889/diff/2#index_header.
(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.
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
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
Let me upload the patch again.
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 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 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)
Attachment #8681769 - Flags: review?(jyavenard) → review+
Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/74726f6aa4e9
https://hg.mozilla.org/mozilla-central/rev/28d11278ff97
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee: nobody → jwwang
You need to log in before you can comment on or make changes to this bug.