Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upOnly trigger a frame build if scene properties have changed. #2927
Conversation
|
r? @kvark and / or @staktrace |
webrender/src/render_backend.rs
Outdated
| @@ -602,12 +602,9 @@ impl RenderBackend { | |||
| } | |||
|
|
|||
| fn process_frame_msg( | |||
| &mut self, | |||
| document_id: DocumentId, | |||
| doc: &mut Document, | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
webrender/src/scene.rs
Outdated
| } | ||
| let mut pending_properties = self.pending_properties | ||
| .take() | ||
| .unwrap_or(DynamicProperties::new()); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| self.float_properties | ||
| .insert(property.key.id, property.value); | ||
| if let Some(pending_properties) = self.pending_properties.take() { | ||
| if pending_properties != self.current_properties { |
This comment has been minimized.
This comment has been minimized.
kvark
Jul 24, 2018
Member
would it be worse to just keep the properties_changed flag and update it in set_properties and update_properties?
This comment has been minimized.
This comment has been minimized.
gw3583
Jul 24, 2018
Author
Collaborator
Within a single transaction, Gecko often has at least one set_properties, followed by an append_properties. It seems more robust to compare after all changes for a transaction have occurred.
webrender_api/src/api.rs
Outdated
| pub struct DynamicProperties { | ||
| pub transforms: Vec<PropertyValue<LayoutTransform>>, | ||
| pub floats: Vec<PropertyValue<f32>>, | ||
| } | ||
|
|
||
| impl DynamicProperties { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In many important use cases (e.g. video playback on youtube) the display list doesn't change very often. Typically, the only thing changing between frames are the natively decoded video texture surfaces. In these cases, we can save a significant amount of CPU usage and battery power by not building a frame at all. Previously, any call to set/update dynamic properties would trigger a frame rebuild. However, it's often the case that Gecko sends dynamic properties updates that are either empty or exactly the same as the previous frame. With this change, WR will compare the scene properties and only trigger a frame rebuild if they have actually changed. This is a partial step towards fixing #2917 - a couple of other changes are also needed to completely avoid a frame rebuiild in this use case.
|
The changes look good to me, thanks! |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Jul 24, 2018
Only trigger a frame build if scene properties have changed. In many important use cases (e.g. video playback on youtube) the display list doesn't change very often. Typically, the only thing changing between frames are the natively decoded video texture surfaces. In these cases, we can save a significant amount of CPU usage and battery power by not building a frame at all. Previously, any call to set/update dynamic properties would trigger a frame rebuild. However, it's often the case that Gecko sends dynamic properties updates that are either empty or exactly the same as the previous frame. With this change, WR will compare the scene properties and only trigger a frame rebuild if they have actually changed. This is a partial step towards fixing #2917 - a couple of other changes are also needed to completely avoid a frame rebuiild in this use case. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2927) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
gw3583 commentedJul 24, 2018
•
edited by larsbergstrom
In many important use cases (e.g. video playback on youtube) the
display list doesn't change very often. Typically, the only
thing changing between frames are the natively decoded video
texture surfaces.
In these cases, we can save a significant amount of CPU usage
and battery power by not building a frame at all.
Previously, any call to set/update dynamic properties would
trigger a frame rebuild. However, it's often the case that
Gecko sends dynamic properties updates that are either empty
or exactly the same as the previous frame.
With this change, WR will compare the scene properties and
only trigger a frame rebuild if they have actually changed.
This is a partial step towards fixing #2917 - a couple of
other changes are also needed to completely avoid a frame
rebuiild in this use case.
This change is