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 upBlock the gstreamer plugin waiting for the next frame #25246
Conversation
|
r? @Manishearth you are our emergency backup gstreamer reviewer. |
| // It's been merged but not yet published. | ||
| // https://github.com/servo/servo/issues/25234 | ||
| let elapsed_micros = self.start.elapsed().as_micros() as u64; | ||
| let frame_duration_micros = self.frame_duration_micros.load(Ordering::SeqCst); |
This comment has been minimized.
This comment has been minimized.
Manishearth
Dec 11, 2019
Member
If this is the only thread doing reads and writes we can just have Acquire/Release everywhere here, I think
This comment has been minimized.
This comment has been minimized.
Manishearth
Dec 11, 2019
Member
actually if it's just one thread then relaxed probably works? unsure.
This comment has been minimized.
This comment has been minimized.
asajeffrey
Dec 11, 2019
Author
Member
All true, I am being conservative here,m I doubt that the different orderings make much difference, given that the next operations are thread::sleep followed by locking a Mutex.
| let frame_duration_micros = | ||
| 1_000_000 * *framerate.denom() as u64 / *framerate.numer() as u64; | ||
| debug!("Setting frame duration to {}micros", frame_duration_micros); | ||
| self.frame_duration_micros |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
asajeffrey
Dec 11, 2019
Author
Member
Renegotiating caps can happen at any time, and the src needs to be Sync, so yes it needs to be an Atomic.
|
approach looks fine, i feel like there's probably a builtin way to do this, and i think the atomics can be better |
|
Really this should be using gstreamers |
| // It's been merged but not yet published. | ||
| // https://github.com/servo/servo/issues/25234 | ||
| let elapsed_micros = self.start.elapsed().as_micros() as u64; | ||
| let frame_duration_micros = self.frame_duration_micros.load(Ordering::SeqCst); |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Dec 11, 2019
Author
Member
All true, I am being conservative here,m I doubt that the different orderings make much difference, given that the next operations are thread::sleep followed by locking a Mutex.
| let frame_duration_micros = | ||
| 1_000_000 * *framerate.denom() as u64 / *framerate.numer() as u64; | ||
| debug!("Setting frame duration to {}micros", frame_duration_micros); | ||
| self.frame_duration_micros |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Dec 11, 2019
Author
Member
Renegotiating caps can happen at any time, and the src needs to be Sync, so yes it needs to be an Atomic.
|
@bors-servo r=Manishearth |
|
|
|
|
asajeffrey
force-pushed the
asajeffrey:gstplugin-framerates
branch
from
c7d8017
to
24678da
|
Rebased. @bors-servo r=Manishearth |
|
|
Block the gstreamer plugin waiting for the next frame <!-- Please describe your changes on the following line: --> Get the GStreamer plugin to produce frames at the requested rate rather than relying on downstream elements to perform throttling. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24833 - [x] These changes do not require tests because <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
@bors-servo retry |
Block the gstreamer plugin waiting for the next frame <!-- Please describe your changes on the following line: --> Get the GStreamer plugin to produce frames at the requested rate rather than relying on downstream elements to perform throttling. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24833 - [x] These changes do not require tests because <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
@bors-servo retry |
Block the gstreamer plugin waiting for the next frame <!-- Please describe your changes on the following line: --> Get the GStreamer plugin to produce frames at the requested rate rather than relying on downstream elements to perform throttling. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24833 - [x] These changes do not require tests because <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
asajeffrey commentedDec 11, 2019
Get the GStreamer plugin to produce frames at the requested rate rather than relying on downstream elements to perform throttling.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors