Closed Bug 1377206 Opened 3 years ago Closed 3 years ago

Set DontThrottle class of service flag on background audio downloads

Categories

(Core :: Networking: HTTP, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

No description provided.
Whiteboard: [necko-active]
Priority: -- → P1
The DontThrottle flag instructs the http backgroung throttling algorithm to never throttle the download.  We may throttle a common responses (with no special flags) when happening in background tabs.  This will prevent background audio to stutter.

Chris, are there any other media loads we should mark with this flag?  We are specifically concerned about background audio like radio.  Should NP (plugin initiated) requests be marked the same way?  Not sure how to recognize they are media, tho.

Thanks!
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Flags: needinfo?(cpearce)
Attachment #8892629 - Flags: review?(cpearce)
Comment on attachment 8892629 [details] [diff] [review]
v1 (media element loading channel explicitly unthrottled)

Review of attachment 8892629 [details] [diff] [review]:
-----------------------------------------------------------------

I think we also need to cover the channel created in ChannelMediaResource::RecreateChannel(), in MediaResource.cpp.
Attachment #8892629 - Flags: review?(cpearce)
Flags: needinfo?(cpearce)
(In reply to Honza Bambas (:mayhemer) from comment #1)
> Should NP
> (plugin initiated) requests be marked the same way?  Not sure how to
> recognize they are media, tho.


I don't know how you could recognize them.
Looks like there are no more channels to mark with this flag.
Attachment #8892629 - Attachment is obsolete: true
Attachment #8892961 - Flags: review?(cpearce)
Depends on: 1387090
Comment on attachment 8892961 [details] [diff] [review]
v2 (media element loading channels explicitly unthrottled)

Review of attachment 8892961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +1199,5 @@
> +        // interaction.
> +        aElement->mUseUrgentStartForChannel = false;
> +      }
> +
> +      // Uncoditionally disable throttling since we want the media fluently

s/Uncoditionally/Unconditionally/
s/the media fluently/the media to fluently/

::: dom/media/MediaResource.cpp
@@ +869,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(mChannel));
> +  if (cos) {
> +    // Uncoditionally disable throttling since we want the media fluently

s/Uncoditionally/Unconditionally/
s/the media fluently/the media to fluently/

Note also that the MediaCache does its own throttling when it thinks we've downloaded "enough" media data.
Attachment #8892961 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #5)
> Note also that the MediaCache does its own throttling when it thinks we've
> downloaded "enough" media data.

Thanks.  This is good to know.  I presume the cache lives on the child process?  How exactly does it throttle?  Just checking since this could be broken with bug 1280629.
Keywords: checkin-needed
(the blocking bug has to land first)
Keywords: checkin-needed
(no, it's not needed!)
No longer depends on: 1387090
Keywords: checkin-needed
(and maybe yes, there could potentially be leaks, rather pushing to try before requesting checkin..)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4031471f84965274750b839759b39da488092aed
Keywords: checkin-needed
OK, safe to go now.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c8cb113321
Explicitly disable throttling of media HTTP responses, r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73c8cb113321
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.