Closed Bug 1426056 Opened 6 years ago Closed 6 years ago

Move "resume downoad" to another thread

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla59

Tracking Flags:

Tracking

Status

firefox59

---

fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

Bug 1426056. P1 - move the decision of resuming download to another thread. 6 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	bechen : review+ mozbugz : review+	Details
Bug 1426056. P2 - remove unused members. 6 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	bechen : review+ mozbugz : review+	Details
Bug 1426056. P3 - assert MediaCacheStream::GetLength() is called off the main thread. 6 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	bechen : review+ mozbugz : review+	Details
Bug 1426056. P4 - fix a case where we don't reopen the channel. 6 years ago JW Wang [:jwwang] [:jw_wang] 59 bytes, text/x-review-board-request	bechen : review+ mozbugz : review+	Details

JW Wang [:jwwang] [:jw_wang]

Assignee

Description

•

6 years ago

https://searchfox.org/mozilla-central/source/dom/media/ChannelMediaResource.cpp#693-705

So we don't call GetLength/GetOffset which need to take the cache monitor on the main thread.

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

6 years ago

Assignee: nobody → jwwang

Blocks: 1369263

Priority: -- → P3

Comment hidden (mozreview-request)

So we won't access mStreamLength/mChannelOffset (which are protected by the
cache monitor) on the main thread.

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

In MediaCacheStream::NotifyResume(), it will not reopen the channel if
|mChannelOffset==mStreamLength && 0 <= mSeekTarget < mStreamLength| while we should.

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

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

6 years ago

Attachment #8937645 - Flags: review?(bechen)

Attachment #8937646 - Flags: review?(bechen)

Attachment #8937647 - Flags: review?(bechen)

Attachment #8937648 - Flags: review?(bechen)

Benjamin Chen [:bechen]

Comment 5

•

6 years ago

mozreview-review

Comment on attachment 8937645 [details]
Bug 1426056. P1 - move the decision of resuming download to another thread.

https://reviewboard.mozilla.org/r/208332/#review214094

Attachment #8937645 - Flags: review?(bechen) → review+

Benjamin Chen [:bechen]

Comment 6

•

6 years ago

mozreview-review

Comment on attachment 8937646 [details]
Bug 1426056. P2 - remove unused members.

https://reviewboard.mozilla.org/r/208334/#review214096

Attachment #8937646 - Flags: review?(bechen) → review+

Benjamin Chen [:bechen]

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8937647 [details]
Bug 1426056. P3 - assert MediaCacheStream::GetLength() is called off the main thread.

https://reviewboard.mozilla.org/r/208336/#review214100

Attachment #8937647 - Flags: review?(bechen) → review+

Benjamin Chen [:bechen]

Comment 8

•

6 years ago

mozreview-review

Comment on attachment 8937648 [details]
Bug 1426056. P4 - fix a case where we don't reopen the channel.

https://reviewboard.mozilla.org/r/208338/#review214102

Attachment #8937648 - Flags: review?(bechen) → review+

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

6 years ago

Attachment #8937645 - Flags: review?(gsquelart)

Attachment #8937646 - Flags: review?(gsquelart)

Attachment #8937647 - Flags: review?(gsquelart)

Attachment #8937648 - Flags: review?(gsquelart)

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

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8937645 [details]
Bug 1426056. P1 - move the decision of resuming download to another thread.

https://reviewboard.mozilla.org/r/208332/#review214112

Attachment #8937645 - Flags: review?(gsquelart) → review+

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

Comment 10

•

6 years ago

mozreview-review

Comment on attachment 8937646 [details]
Bug 1426056. P2 - remove unused members.

https://reviewboard.mozilla.org/r/208334/#review214114

Attachment #8937646 - Flags: review?(gsquelart) → review+

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

Comment 11

•

6 years ago

mozreview-review

Comment on attachment 8937647 [details]
Bug 1426056. P3 - assert MediaCacheStream::GetLength() is called off the main thread.

https://reviewboard.mozilla.org/r/208336/#review214116

Attachment #8937647 - Flags: review?(gsquelart) → review+

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

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8937648 [details]
Bug 1426056. P4 - fix a case where we don't reopen the channel.

https://reviewboard.mozilla.org/r/208338/#review214120

Attachment #8937648 - Flags: review?(gsquelart) → review+

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 13

•

6 years ago

Thanks for the reviews!

Mozilla Autoland

Comment 14

•

6 years ago

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 4 changesets with 9 changes to 4 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev ad78719a8bdb needs "Bug N" or "No bug" in the commit message.
remote: JW Wang <jwwang@mozilla.com>
remote: Buy 1426056. P4 - fix a case where we don't reopen the channel. r=bechen,gerald
remote: 
remote: In MediaCacheStream::NotifyResume(), it will not reopen the channel if
remote: |mChannelOffset==mStreamLength && 0 <= mSeekTarget < mStreamLength| while we should.
remote: 
remote: MozReview-Commit-ID: 2knkgy6FEVw
remote: *************************************************************
remote: 
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev f4d4e6c1feed needs "Bug N" or "No bug" in the commit message.
remote: JW Wang <jwwang@mozilla.com>
remote: Buy 1426056. P3 - assert MediaCacheStream::GetLength() is called off the main thread. r=bechen,gerald
remote: 
remote: MozReview-Commit-ID: 7L8C4f2Lg3C
remote: *************************************************************
remote: 
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 7f9aa892d403 needs "Bug N" or "No bug" in the commit message.
remote: JW Wang <jwwang@mozilla.com>
remote: Buy 1426056. P2 - remove unused members. r=bechen,gerald
remote: 
remote: MozReview-Commit-ID: 6CjxrFzutkP
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment hidden (mozreview-request)

Comment on attachment 8937646 [details]
Bug 1426056. P2 - remove unused members.

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

Comment hidden (mozreview-request)

Comment on attachment 8937647 [details]
Bug 1426056. P3 - assert MediaCacheStream::GetLength() is called off the main thread.

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

Comment hidden (mozreview-request)

Comment on attachment 8937648 [details]
Bug 1426056. P4 - fix a case where we don't reopen the channel.

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

Pulsebot

Comment 18

•

6 years ago

Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6682e9d24639
P1 - move the decision of resuming download to another thread. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/e3ca1814e047
P2 - remove unused members. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/d1e6a2a527c1
P3 - assert MediaCacheStream::GetLength() is called off the main thread. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/7b78e22bb69e
P4 - fix a case where we don't reopen the channel. r=bechen,gerald

Eliza Balazs [:ebalazs_]

Comment 19

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/6682e9d24639
https://hg.mozilla.org/mozilla-central/rev/e3ca1814e047
https://hg.mozilla.org/mozilla-central/rev/d1e6a2a527c1
https://hg.mozilla.org/mozilla-central/rev/7b78e22bb69e

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox59: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

You need to log in before you can comment on or make changes to this bug.