New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve implementation of media resource fetch algorithm #22433

Merged
merged 3 commits into from Dec 13, 2018

Conversation

Projects
None yet
6 participants
@ferjm
Copy link
Member

ferjm commented Dec 12, 2018

I have been observing inconsistent behaviors with my local tests depending of the media asset being played. I figured that we had these issues:

  • We were setting the player EOS as soon as we got the first process_response_eof. This is fine only if there is a single request. But that's not the case for multiple range requests or for seeks. Setting the player EOS makes the player appsrc reject any new buffers, and that breaks playback. Figuring out when is the right time to set the player EOS won't be a straight forward task, so my suggested fix for now is to simply not set it for now. It is a cleanup step that it would be nice to have but it is not mandatory.

  • We were setting the input size more than once for multiple range requests and with the incorrect value. The fix uses the content-length or the content-range headers for single and range requests respectively.

  • We were moving to the HaveEnoughData state if a fetch request succeded but no data was fetched from the network.

  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 12, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmlmediaelement.rs
  • @KiChjang: components/script/dom/htmlmediaelement.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Dec 12, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@ferjm ferjm referenced this pull request Dec 12, 2018

Open

Set media player EOS #22434

1 of 2 tasks complete
@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

I filed #22434 for setting the EOS back

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

r? @ceyusa

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Trying commit 243b4e1 with merge abc5e3c...

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22433 - ferjm:player.eos.size, r=<try>
Do not set player EOS. Set correct input size and only if it changes

I have been observing inconsistent behaviors with my local tests depending of the media asset being played. I figured that we had two issues:

- We were setting the player EOS as soon as we got the first [process_response_eof](https://github.com/servo/servo/blob/1046ae58a155d3f1ab4d011242a03a81a712f3c4/components/script/dom/htmlmediaelement.rs#L1596). This is fine only if there is a single request. But that's not the case for multiple range requests or for seeks. Setting the player EOS makes the player appsrc reject any new buffers, and that breaks playback. Figuring out when is the right time to set the player EOS won't be a straight forward task, so my suggested fix for now is to simply not set it for now. It is a cleanup step that it would be nice to have but it is not mandatory.

- We were setting the input size more than once for multiple range requests and with the incorrect value. The fix uses the `content-length` or the `content-range` headers for single and range requests respectively.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22433)
<!-- Reviewable:end -->
@ceyusa

This comment has been minimized.

Copy link
Contributor

ceyusa commented Dec 12, 2018

r+

@ferjm ferjm referenced this pull request Dec 12, 2018

Merged

Implement Ended media attribute #22348

3 of 3 tasks complete
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

💔 Test failed - linux-rel-css

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22433 - ferjm:player.eos.size, r=<try>
Do not set player EOS. Set correct input size and only if it changes

I have been observing inconsistent behaviors with my local tests depending of the media asset being played. I figured that we had two issues:

- We were setting the player EOS as soon as we got the first [process_response_eof](https://github.com/servo/servo/blob/1046ae58a155d3f1ab4d011242a03a81a712f3c4/components/script/dom/htmlmediaelement.rs#L1596). This is fine only if there is a single request. But that's not the case for multiple range requests or for seeks. Setting the player EOS makes the player appsrc reject any new buffers, and that breaks playback. Figuring out when is the right time to set the player EOS won't be a straight forward task, so my suggested fix for now is to simply not set it for now. It is a cleanup step that it would be nice to have but it is not mandatory.

- We were setting the input size more than once for multiple range requests and with the incorrect value. The fix uses the `content-length` or the `content-range` headers for single and range requests respectively.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22433)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Trying commit 8b99e97 with merge 1fffeb7...

@ceyusa

This comment has been minimized.

Copy link
Contributor

ceyusa commented Dec 12, 2018

r+

\o/

@ferjm ferjm changed the title Do not set player EOS. Set correct input size and only if it changes Improve implementation of media resource fetch algorithm Dec 12, 2018

@ferjm ferjm force-pushed the ferjm:player.eos.size branch from 8b99e97 to 8818898 Dec 12, 2018

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

It seems that try is happy.

@bors-servo r=ceyusa

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

📌 Commit 8818898 has been approved by ceyusa

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Testing commit 8818898 with merge cc982b5...

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22433 - ferjm:player.eos.size, r=ceyusa
Improve implementation of media resource fetch algorithm

I have been observing inconsistent behaviors with my local tests depending of the media asset being played. I figured that we had these issues:

- We were setting the player EOS as soon as we got the first [process_response_eof](https://github.com/servo/servo/blob/1046ae58a155d3f1ab4d011242a03a81a712f3c4/components/script/dom/htmlmediaelement.rs#L1596). This is fine only if there is a single request. But that's not the case for multiple range requests or for seeks. Setting the player EOS makes the player appsrc reject any new buffers, and that breaks playback. Figuring out when is the right time to set the player EOS won't be a straight forward task, so my suggested fix for now is to simply not set it for now. It is a cleanup step that it would be nice to have but it is not mandatory.

- We were setting the input size more than once for multiple range requests and with the incorrect value. The fix uses the `content-length` or the `content-range` headers for single and range requests respectively.

- We were moving to the HaveEnoughData state if a fetch request succeded but no data was fetched from the network.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22433)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 12, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Testing commit 8818898 with merge eab848d...

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22433 - ferjm:player.eos.size, r=ceyusa
Improve implementation of media resource fetch algorithm

I have been observing inconsistent behaviors with my local tests depending of the media asset being played. I figured that we had these issues:

- We were setting the player EOS as soon as we got the first [process_response_eof](https://github.com/servo/servo/blob/1046ae58a155d3f1ab4d011242a03a81a712f3c4/components/script/dom/htmlmediaelement.rs#L1596). This is fine only if there is a single request. But that's not the case for multiple range requests or for seeks. Setting the player EOS makes the player appsrc reject any new buffers, and that breaks playback. Figuring out when is the right time to set the player EOS won't be a straight forward task, so my suggested fix for now is to simply not set it for now. It is a cleanup step that it would be nice to have but it is not mandatory.

- We were setting the input size more than once for multiple range requests and with the incorrect value. The fix uses the `content-length` or the `content-range` headers for single and range requests respectively.

- We were moving to the HaveEnoughData state if a fetch request succeded but no data was fetched from the network.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22433)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 13, 2018

@bors-servo bors-servo merged commit 8818898 into servo:master Dec 13, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Dec 13, 2018

Merged

Implement HTMLMediaElement poster attribute #22399

4 of 4 tasks complete

@ferjm ferjm deleted the ferjm:player.eos.size branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment