You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "jmuehlner (via GitHub)" <gi...@apache.org> on 2023/06/07 17:38:03 UTC
[GitHub] [guacamole-client] jmuehlner opened a new pull request, #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
jmuehlner opened a new pull request, #883:
URL: https://github.com/apache/guacamole-client/pull/883
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1221963793
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -139,22 +146,60 @@ Guacamole.SessionRecording = function SessionRecording(source) {
var currentFrame = -1;
/**
- * The timestamp of the frame when playback began, in milliseconds. If
- * playback is not in progress, this will be null.
+ * True if the player is currently playing, or false otherwise.
*
* @private
- * @type {number}
+ * @type {boolean}
+ */
+ var currentlyPlaying = null;
+
+ /**
+ * The current position within the recording, in milliseconds.
+ *
+ * @private
+ * @type {!number}
+ */
+ var currentPosition = 0;
+
+ /**
+ * Updates the position of the recording if it's playing, incrementing the
+ * value by the provided refreshInterval, and calling the onseek handler
+ * with the new value, if a handler was was provided.
*/
- var startVideoTimestamp = null;
+ function refreshPosition() {
Review Comment:
I originally implemented this with the interval happening in the recording player in the angular app (`player.js` below) but it was a mess. Having to keep multiple different sources of truth led to a lot of potential inconsistency, with issues like the interval advancing the position in the player past the start of the next frame, leading to a jittering progress bar.
Keeping the source of truth inside the recording itself allows for easy checking to make sure that the position monotonically increases during playback, with no weird behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228560706
##########
extensions/guacamole-history-recording-storage/src/main/java/org/apache/guacamole/history/connection/HistoryConnectionRecord.java:
##########
@@ -131,7 +131,9 @@ public HistoryConnectionRecord(ConnectionRecord record) throws GuacamoleExceptio
*/
private boolean isSessionRecording(File file) {
- try (Reader reader = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) {
Review Comment:
Ok, so apparently this try-with-resources language feature doesn't handle exceptions very well. I had a garbage recording in my recording storage, causing the `readInstruction()` call below to throw a distinctly unfriendly error:
```
17:32:58.156 [http-nio-8080-exec-2] ERROR o.a.g.rest.RESTExceptionMapper - Unexpected internal error: Self-suppression not permitted
17:32:58.161 [http-nio-8080-exec-2] DEBUG o.a.g.rest.RESTExceptionMapper - Unexpected error in REST endpoint.
java.lang.IllegalArgumentException: Self-suppression not permitted
at java.base/java.lang.Throwable.addSuppressed(Throwable.java:1054)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.isSessionRecording(HistoryConnectionRecord.java:140)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.getType(HistoryConnectionRecord.java:201)
[ snipped many lines for readability ]
Caused by: org.apache.guacamole.GuacamoleServerException: Non-numeric character in element length.
at org.apache.guacamole.io.ReaderGuacamoleReader.read(ReaderGuacamoleReader.java:157)
at org.apache.guacamole.io.ReaderGuacamoleReader.readInstruction(ReaderGuacamoleReader.java:195)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.isSessionRecording(HistoryConnectionRecord.java:137)
... 64 common frames omitted
```
This is related to the suppressed exceptions discussed here https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html#suppressed-exceptions.
I don't fully understand exactly what makes the `GuacamoleServerException` a self-suppressed exception, and why that's not permitted, but in any case, going back to this more traditional approach solves the problem.
I now get a sensible debug message in the logs about the problem with the recording and the page loads fine, as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228410515
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
So to be clear - you're suggesting that we modify `seekToFrame` to either advance to the next frame _or_ increment `currentPosition` depending on whether `currentPosition + refreshInterval` >= the timestamp of the next frame?
I think that could make sense. Let me see if it's simpler than what I have now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228651045
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
~This change seems to have caused some weird jumping effects on pause/resume. Drafting while I investigate.~
Problem fixed - undrafted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228420327
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Or rather, not the `seekToFrame` function iself, but perhaps a wrapper function here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228552549
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Ok, how does this look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228724468
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
> How do my latest changes look? That's more or less what I'm doing, though I didn't do do the multiple-of-`refreshInterval` thing, I just seek to the next `refreshInterval`, or the next frame, whichever is sooner.
I think you might need to round down to the nearest multiple-of-`refreshInterval`. Seeking to the next `refreshInterval` would be relative to the current position, which may not be correct. For example (timestamps below are relative to start of recording):
1. Frame at 0ms. Player shows "00:00".
2. Frame at 400ms. Player shows "00:00".
3. Frame at 10000ms. Player waits until current position + `refreshInterval`, showing "00:01" at 1400ms.
Here, the player took 1.4 seconds of playback before the visible timestamp progressed from "00:00" to "00:01". If this sort of issue occurred multiple times in the recording, it would appear to the user as a jittering playback timestamp, incrementing at inconsistent intervals.
With rounding down to the nearest multiple of `refreshInterval`, we instead get:
1. Frame at 0ms. Player shows "00:00".
2. Frame at 400ms. Player shows "00:00".
3. Frame at 10000ms. Player waits until the nearest multiple of `refreshInterval`, showing "00:01" at 1000ms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1231415173
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -678,20 +727,62 @@ Guacamole.SessionRecording = function SessionRecording(source) {
*/
var continuePlayback = function continuePlayback() {
+ // Do not continue playback if the recording is paused
+ if (!recording.isPlaying())
+ return;
+
// If frames remain after advancing, schedule next frame
if (currentFrame + 1 < frames.length) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // The position at which the next frame should be rendered
+ var nextFramePosition = toRelativeTimestamp(next.timestamp);
Review Comment:
Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228387914
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Might we be able to avoid `refreshIntervalExecutor` entirely by using `refreshInterval` as the upper bound for `delay` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228591753
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -32,8 +32,13 @@ var Guacamole = Guacamole || {};
* @param {!Blob|Guacamole.Tunnel} source
* The Blob from which the instructions of the recording should
* be read.
+ * @param {number} refreshInterval
+ * The minimum number of milliseconds between updates to the recording
+ * position through the provided onseek() callback. If non-positive, this
+ * parameter will be ignored, and the recording position will only be
+ * updated when seek requests are made, or when new frames are rendered.
Review Comment:
What are your thoughts on defaulting `refreshInterval` to 1000?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228628870
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
How do my latest changes look? That's more or less what I'm doing, though I didn't do do the multiple-of-`refreshInterval` thing, I just seek to the next `refreshInterval`, or the next frame, whichever is sooner.
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -32,8 +32,13 @@ var Guacamole = Guacamole || {};
* @param {!Blob|Guacamole.Tunnel} source
* The Blob from which the instructions of the recording should
* be read.
+ * @param {number} refreshInterval
+ * The minimum number of milliseconds between updates to the recording
+ * position through the provided onseek() callback. If non-positive, this
+ * parameter will be ignored, and the recording position will only be
+ * updated when seek requests are made, or when new frames are rendered.
Review Comment:
Sure, fine by me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228789838
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Ok, how does this look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228420327
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Or rather, not the `seekToFrame` function itself, but perhaps a wrapper function here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1231198868
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -211,6 +219,22 @@ Guacamole.SessionRecording = function SessionRecording(source) {
*/
var seekCallback = null;
+ /**
+ * Any current timeout associated with scheduling frame replay, or updating
+ * the current position, or null if no frame position increment is currently
+ * scheduled.
+ *
+ * @private
+ * @type {number}
+ */
+ var updateTimeout = null;
+
+ /**
+ * The browser timestamp of the last time that currentPosition was updated
+ * while playing, or null if the recording is not currently playing.
+ */
+ var lastUpdateTimestamp = null;
Review Comment:
This should be annotated `@private` and with its proper `@type`.
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -32,8 +32,18 @@ var Guacamole = Guacamole || {};
* @param {!Blob|Guacamole.Tunnel} source
* The Blob from which the instructions of the recording should
* be read.
+ * @param {number} refreshInterval
+ * The minimum number of milliseconds between updates to the recording
+ * position through the provided onseek() callback. If non-positive, this
+ * parameter will be ignored, and the recording position will only be
+ * updated when seek requests are made, or when new frames are rendered.
+ * If not specified, refreshInterval will default to 1000 milliseconds.
Review Comment:
The notation for an optional parameter with a default value would be:
```
@param {number} [refreshInterval=1000]
```
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -678,20 +727,62 @@ Guacamole.SessionRecording = function SessionRecording(source) {
*/
var continuePlayback = function continuePlayback() {
+ // Do not continue playback if the recording is paused
+ if (!recording.isPlaying())
+ return;
+
// If frames remain after advancing, schedule next frame
if (currentFrame + 1 < frames.length) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // The position at which the next frame should be rendered
+ var nextFramePosition = toRelativeTimestamp(next.timestamp);
Review Comment:
The calculation of the next timestamp based the amount of real time that passed since playback started was intended to correct for timing error that may accumulate if frames take longer to render than when the recording was made. For example, if a series of 100 frames took 10ms each when recorded, but are taking 20ms in the browser currently playing that recording back, that ~1 second of original time will take ~2 seconds to play back and will offset the rest of the recording unless steps are taken to correct this on the fly.
There is also occasional scheduling error due purely to inaccuracy in JS timer events, and corrective steps need to be taken to prevent that error from accumulating to the point that the recording is significantly delayed.
It looks to me like the new code will schedule frame rendering based purely on the positions of the frames, without taking delays due to processing into account. Am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228671911
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
Ok, resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228651045
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
This change seems to have caused some weird jumping effects on pause/resume. Drafting while I investigate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1231373713
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -678,20 +727,62 @@ Guacamole.SessionRecording = function SessionRecording(source) {
*/
var continuePlayback = function continuePlayback() {
+ // Do not continue playback if the recording is paused
+ if (!recording.isPlaying())
+ return;
+
// If frames remain after advancing, schedule next frame
if (currentFrame + 1 < frames.length) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // The position at which the next frame should be rendered
+ var nextFramePosition = toRelativeTimestamp(next.timestamp);
Review Comment:
Ok, how does this look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228560706
##########
extensions/guacamole-history-recording-storage/src/main/java/org/apache/guacamole/history/connection/HistoryConnectionRecord.java:
##########
@@ -131,7 +131,9 @@ public HistoryConnectionRecord(ConnectionRecord record) throws GuacamoleExceptio
*/
private boolean isSessionRecording(File file) {
- try (Reader reader = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) {
Review Comment:
Ok, so apparently this try-with-resources language feature doesn't handle exceptions very well. I had a garbage recording in my recording storage, causing the `readInstruction()` call below to throw a distinctly unfriendly error:
```
17:32:58.156 [http-nio-8080-exec-2] ERROR o.a.g.rest.RESTExceptionMapper - Unexpected internal error: Self-suppression not permitted
17:32:58.161 [http-nio-8080-exec-2] DEBUG o.a.g.rest.RESTExceptionMapper - Unexpected error in REST endpoint.
java.lang.IllegalArgumentException: Self-suppression not permitted
at java.base/java.lang.Throwable.addSuppressed(Throwable.java:1054)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.isSessionRecording(HistoryConnectionRecord.java:140)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.getType(HistoryConnectionRecord.java:201)
[ snipped many lines for readability ]
Caused by: org.apache.guacamole.GuacamoleServerException: Non-numeric character in element length.
at org.apache.guacamole.io.ReaderGuacamoleReader.read(ReaderGuacamoleReader.java:157)
at org.apache.guacamole.io.ReaderGuacamoleReader.readInstruction(ReaderGuacamoleReader.java:195)
at org.apache.guacamole.history.connection.HistoryConnectionRecord.isSessionRecording(HistoryConnectionRecord.java:137)
... 64 common frames omitted
```
This is related to the suppressed exceptions discussed here https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html#suppressed-exceptions.
I don't fully understand exactly what makes the `GuacamoleServerException` a self-suppressed exception, and why that's bad, but in any case, going back to this more traditional approach solves the problem.
I now get a sensible debug message in the logs about the problem with the recording and the page loads fine, as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1228588817
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -684,14 +766,13 @@ Guacamole.SessionRecording = function SessionRecording(source) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // Add the offset to the next frame timestamp to calculate the delay
+ var delay = toRelativeTimestamp(next.timestamp) - currentPosition;
Review Comment:
> So to be clear - you're suggesting that we modify `seekToFrame` to either advance to the next frame _or_ increment `currentPosition` depending on whether `currentPosition + refreshInterval` >= the timestamp of the next frame?
More or less. My thinking was:
* The core issue is that the previous code scheduled the next seek based on the frame duration, which can be significantly longer than one second.
* Part of these changes is ensuring we can reliably seek to any position, even between frames.
* We might be able to achieve the desired effect by ensuring the delay until next seek is bounded at the refresh interval, even if that seek would be mid-frame.
I'd think that would mean we'd have to:
1. Calculate the timestamp of the nearest whole multiple-of-`refreshInterval`.
2. If the next frame is before that, easy - seek to _that_.
3. If not, seek to the calculated timestamp.
That way we are guaranteed to be updated every `refreshInterval` relative to playback position, not relative to a local timer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #883:
URL: https://github.com/apache/guacamole-client/pull/883#discussion_r1231280310
##########
guacamole-common-js/src/main/webapp/modules/SessionRecording.js:
##########
@@ -678,20 +727,62 @@ Guacamole.SessionRecording = function SessionRecording(source) {
*/
var continuePlayback = function continuePlayback() {
+ // Do not continue playback if the recording is paused
+ if (!recording.isPlaying())
+ return;
+
// If frames remain after advancing, schedule next frame
if (currentFrame + 1 < frames.length) {
// Pull the upcoming frame
var next = frames[currentFrame + 1];
- // Calculate the real timestamp corresponding to when the next
- // frame begins
- var nextRealTimestamp = next.timestamp - startVideoTimestamp + startRealTimestamp;
+ // The position at which the next frame should be rendered
+ var nextFramePosition = toRelativeTimestamp(next.timestamp);
Review Comment:
Oops, yeah I did indeed remove that. It was unclear at the time why it was needed, but that makes sense. I'll put it back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper merged pull request #883: GUACAMOLE-1803: Update session recording playback at least once per second and allow arbitrary seeking.
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper merged PR #883:
URL: https://github.com/apache/guacamole-client/pull/883
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org