Closed Bug 1348053 Opened 8 years ago Closed 8 years ago

Channel loading content for click-to-play media should be marked as urgent-start

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mayhemer, Assigned: jwwang)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1348053 - add the UrgentStart flag to the channel when the loading is initiated by a user interaction for quicker network response.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
The idea is to start playing the media ASAP user clicks the play button.  The urgent-start classification will tell the network requests scheduler to perform it with the highest priority and also ignoring any parallelism or overall connection limits.
(In reply to Honza Bambas (:mayhemer) from comment #0)
> The idea is to start playing the media ASAP user clicks the play button. 
> The urgent-start classification will tell the network requests scheduler to
> perform it with the highest priority and also ignoring any parallelism or
> overall connection limits.

I think maybe we can leverage some help from the media team at Taipei.

@Blake, is it possible to find someone in your team to work on this? Thanks.
Flags: needinfo?(bwu)
(In reply to Honza Bambas (:mayhemer) from comment #0)
> The idea is to start playing the media ASAP user clicks the play button. 
Is the user interaction required or do we also want to boost the priority when the script calls play() on a media element?
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #0)
> The urgent-start classification will tell the network requests scheduler to
> perform it with the highest priority and also ignoring any parallelism or
> overall connection limits.
Any sample code showing how to do that?
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > The urgent-start classification will tell the network requests scheduler to
> > perform it with the highest priority and also ignoring any parallelism or
> > overall connection limits.
> Any sample code showing how to do that?

Please see this:
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/test/unit/test_bug1312774_http1.js#61-64
It would be good to have c++ code sample as well.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > The idea is to start playing the media ASAP user clicks the play button. 
> Is the user interaction required or do we also want to boost the priority
> when the script calls play() on a media element?

I think to keep this simple, it's probably OK to just handle clicking the play button directly.  

If it's simple enough to detect also play() invoked by a script that handles a user input (e.g. like clicking a button assigned onclick event) then it might be nice too (a followup bug would be sufficient for it, tho).


(In reply to Kershaw Chang [:kershaw] from comment #4)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> > (In reply to Honza Bambas (:mayhemer) from comment #0)
> > > The urgent-start classification will tell the network requests scheduler to
> > > perform it with the highest priority and also ignoring any parallelism or
> > > overall connection limits.
> > Any sample code showing how to do that?
> 
> Please see this:
> http://searchfox.org/mozilla-central/rev/
> b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/test/unit/
> test_bug1312774_http1.js#61-64

Yes, it's just about adding the nsIClassOfService.UrgentStart flag on the channel when it implements the nsIClassOfService interface.  Thanks Kershaw.
Flags: needinfo?(honzab.moz)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> It would be good to have c++ code sample as well.

#include "nsIClassOfService.h"

...
nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
if (cos) {
  cos->AddClassFlags(nsIClassOfService::UrgentStart);
}

that's it.  the |channel| is the channel you create with some of the NS_NewChannel*() or similar.  The class (the flag) has to be set before you AsyncOpen() that channel.
Thanks for the explanation. I still have some questions to clarify:

1. The media element preloads media data even before the user clicks the play button or play() is called by the script such that playback can start as soon as possible when requested. So the UrgentStart flag doesn't really help in this case.

2. http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/protocol/http/HSTSPrimerListener.cpp#313
We have this example that wants to minimize blocking the page load event. Media element will also block the page load event when the preload attribute is set to 'auto' or 'metadata'. So it makes sense to assign the UrgentStart flag to the channel at the very beginning of the media load algorithm.

3. If the preload attribute is 'none', the media element will not start playback until play() is called (either by script or user interaction through video control). This case can also be reduced to case 2 where we want to assign the UrgentStart flag at the very beginning of the media load algorithm.

4. Do we have to clear the flag once playback is progressed smoothly? Does it have any negative impact if the flag is not cleared for the whole playback session?
To be more clear about 3:
If the preload attribute is 'none', the media element will not download any data until play() is called.
First I will rather restate what the urgent-start flag is purposed for.  Certain user interaction (clicking a link, clicking play buttons, clicking "zoom image" button) involve a network request.  User cannot see a reaction from the browser until the response is coming back - at least the first bytes.  Where the connection limit is exhausted and the request put to a network queue, it may take a while (seconds) to get it on the wire - UX suffers from that.  The urgent-start classification says we want to disobey these limits and send the request nonetheless on a new connection if needed.

Hence, the urgent-start class should only be used for requests resulting from direct user interaction.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> Thanks for the explanation. I still have some questions to clarify:
> 
> 1. The media element preloads media data even before the user clicks the
> play button or play() is called by the script such that playback can start
> as soon as possible when requested. So the UrgentStart flag doesn't really
> help in this case.

For the preload it probably doesn't.  But for the load invoked by the play button I believe it does.

> 
> 2.
> http://searchfox.org/mozilla-central/rev/
> b8cce081200129a1307e2a682f121135b5ca1d19/netwerk/protocol/http/
> HSTSPrimerListener.cpp#313
> We have this example that wants to minimize blocking the page load event.

Not the page load event.  The prime requests blocks a particular resource load, sometime a render blocking one.  Hence, the prime request should get a response ASAP.  Use of the class at this particular spot is not a perfect example.  I've added just to minimize the performance impact priming has.  It's all may be subject to change.

> Media element will also block the page load event when the preload attribute
> is set to 'auto' or 'metadata'. So it makes sense to assign the UrgentStart
> flag to the channel at the very beginning of the media load algorithm.

No, not for this case.  Normal prioritization should be used if the content is expected to be "important".  Yes, hard to decide.  Definitely not something we should deal with in this bug.

> 
> 3. If the preload attribute is 'none', the media element will not start
> playback until play() is called (either by script or user interaction
> through video control). This case can also be reduced to case 2 where we
> want to assign the UrgentStart flag at the very beginning of the media load
> algorithm.
> 
> 4. Do we have to clear the flag once playback is progressed smoothly? Does
> it have any negative impact if the flag is not cleared for the whole
> playback session?

No need to clear the flag on an existing and already open channel.  The flag only has effect on the way we initially schedule the request, but nothing else.  Although, if you open a new channel e.g. as a reaction to seek, then urgent-start would be good, as it's a user interaction.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #9)
> To be more clear about 3:
> If the preload attribute is 'none', the media element will not download any
> data until play() is called.

OK, then setting urgent-start when play() is called might be good, ideally when it's invoked by user.
Priority: -- → P2
Here is the summary of above discussions. Hope I get it clear and right:
1. For media elements with preload='auto' or 'metadata': don't add the UrgentStart flag.
   Since network download starts before user click, it is pointless to add the flag in this case.
2. For media elements with preload='none', add the UrgentStart flag when play() is triggered by user interaction so we can have better responsiveness to the user.
Flags: needinfo?(bwu)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> Here is the summary of above discussions. Hope I get it clear and right:
> 1. For media elements with preload='auto' or 'metadata': don't add the
> UrgentStart flag.
>    Since network download starts before user click, it is pointless to add
> the flag in this case.
> 2. For media elements with preload='none', add the UrgentStart flag when
> play() is triggered by user interaction so we can have better responsiveness
> to the user.

Yes, sounds right to me!  Thanks.
Comment on attachment 8856788 [details]
Bug 1348053 - add the UrgentStart flag to the channel when the loading is initiated by a user interaction for quicker network response.

Cancel reviews.
Attachment #8856788 - Flags: review?(cpearce)
Assign to JW who is working on this, thanks!
Assignee: nobody → jwwang
Tag this bug [qf] since it may improve user perceived responsiveness and performance.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Somehow adding the UrgentStart flag prevents HTMLMediaElement::MediaLoadListener::OnStartRequest() [1] from being called which I believe is a network stack issue.

Hi  Shian-Yow,
Can you find someone to debug the issue mentioned above? Thanks!

The repro steps is simple:
1. apply the patches.
2. run the test case added in P2.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/html/HTMLMediaElement.cpp#474
Flags: needinfo?(swu)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #19)
> Somehow adding the UrgentStart flag prevents
> HTMLMediaElement::MediaLoadListener::OnStartRequest() [1] from being called
> which I believe is a network stack issue.
> 
> Hi  Shian-Yow,
> Can you find someone to debug the issue mentioned above? Thanks!
> 
> The repro steps is simple:
> 1. apply the patches.
> 2. run the test case added in P2.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/html/HTMLMediaElement.cpp#474

Kershaw will take a look at it.
Flags: needinfo?(swu) → needinfo?(kechang)
(In reply to Shian-Yow Wu [:swu] from comment #20)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #19)
> > Somehow adding the UrgentStart flag prevents
> > HTMLMediaElement::MediaLoadListener::OnStartRequest() [1] from being called
> > which I believe is a network stack issue.
> > 
> > Hi  Shian-Yow,
> > Can you find someone to debug the issue mentioned above? Thanks!
> > 
> > The repro steps is simple:
> > 1. apply the patches.
> > 2. run the test case added in P2.
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/html/HTMLMediaElement.cpp#474
> 
> Kershaw will take a look at it.

I found the problem is at [1]. There is a terrible mistake (I made) of counting the available new connection.
I'll file a bug to fix this.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/netwerk/protocol/http/nsHttpConnectionMgr.cpp#1100
Flags: needinfo?(kechang)
Depends on: 1355700
Are there measurements somewhere that show that setting the UrgentStart flag actually helps?

Regarding the summary of the conversation so far:

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> Here is the summary of above discussions. Hope I get it clear and right:
> 1. For media elements with preload='auto' or 'metadata': don't add the
> UrgentStart flag.
>    Since network download starts before user click, it is pointless to add
> the flag in this case.
> 2. For media elements with preload='none', add the UrgentStart flag when
> play() is triggered by user interaction so we can have better responsiveness
> to the user.

Also this comment by Honza:

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Although, if you open a new channel e.g. as a reaction to seek, then
> urgent-start would be good, as it's a user interaction.

I agree it would be useful to use the UrgentStart flag in response to a seek. That can be a separate bug.
Comment on attachment 8856788 [details]
Bug 1348053 - add the UrgentStart flag to the channel when the loading is initiated by a user interaction for quicker network response.

https://reviewboard.mozilla.org/r/128710/#review131868

r+ assuming that dependent bug is fixed, and assuming this actually helps.

::: dom/html/HTMLMediaElement.cpp:501
(Diff revision 2)
>  
>    // Don't continue to load if the request failed or has been canceled.
>    nsresult status;
>    nsresult rv = aRequest->GetStatus(&status);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  printf("JW: OnStartRequest status=%x\n", int(status));

How about using a MOZ_LOG here instead?
Attachment #8856788 - Flags: review?(cpearce) → review+
Comment on attachment 8856788 [details]
Bug 1348053 - add the UrgentStart flag to the channel when the loading is initiated by a user interaction for quicker network response.

https://reviewboard.mozilla.org/r/128710/#review131868

> How about using a MOZ_LOG here instead?

Oops! The log is for debugging and should be removed.
Priority: P2 → P1
Attachment #8857310 - Attachment is obsolete: true
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32ae2383a6d4
add the UrgentStart flag to the channel when the loading is initiated by a user interaction for quicker network response. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/32ae2383a6d4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: