Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
corentin-soriano commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2380553947 > > I haven't had any problems with RDP/VNC. In terminal, cat a large file is very slow and can in some cases disconnect me from guacamole and the display is not restored when exiting the alternate buffer. > > > > @corentin-soriano Thanks for checking this. The remaining performance trouble with the terminal seems rooted in GUACAMOLE-1256, so if there isn't an obvious fix, I think we'll just end up rolling back those changes and revisiting next release. The changes here should no longer affect that. I misunderstood your comment in the ticket, I thought you had completely replaced the display_flush and that the problem was solved. In this case, what seems best is to remove it in the scroll_up function while keeping it in scroll_down. The latencies would no longer be present, and the scrolling bug in vi would also be gone. I will create the PR on Monday. Thanks for the clarification. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779377190
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,271 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of the given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of the given rectangle
+ * should be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of the given rectangle within the given immutable
+ * (const) buffer, where the upper-left corner of the given buffer is (0, 0).
+ * If the memory address cannot be calculated because doing so would overflow
+ * the maximum value of a size_t, execution of the current process is
+ * automatically aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The const buffer within which the address of the given rectangle should
+ * be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_CONST_BUFFER(rect, buffer, stride, bpp) ((const void*) (\
+((const unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+struct guac_rect {
+
+/**
+ * The X coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer X coordinate that is part of
+ * this rectangle, with greater integer X coordinates being part of this
+ * rectangle up to but excluding the right boundary.
+ *
+ * This value MUST be less than or equal to the right boundary. If this
+ * value is equal to the right boundary, the rectangle is empty (has no
+ * width).
+ */
+int left;
+
+/**
+ * The Y coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer Y coordinate that is part of
+ * this rectangle, with greater integer Y coordinates being part of this
+ * rectangle up to but excluding the bottom boundary.
+ *
+ * This value MUST be less than or equal to the bottom boundary.
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779376790 ## src/libguac/display-render-thread.c: ## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "display-priv.h" +#include "guacamole/client.h" +#include "guacamole/display.h" +#include "guacamole/flag.h" +#include "guacamole/mem.h" +#include "guacamole/timestamp.h" + +/** + * The maximum duration of a frame in milliseconds. This ensures we at least + * meet a reasonable minimum framerate in the case that the RDP server provides Review Comment: Oops, yep. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779376850
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,271 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of the given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of the given rectangle
+ * should be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of the given rectangle within the given immutable
+ * (const) buffer, where the upper-left corner of the given buffer is (0, 0).
+ * If the memory address cannot be calculated because doing so would overflow
+ * the maximum value of a size_t, execution of the current process is
+ * automatically aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The const buffer within which the address of the given rectangle should
+ * be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_CONST_BUFFER(rect, buffer, stride, bpp) ((const void*) (\
+((const unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+struct guac_rect {
+
+/**
+ * The X coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer X coordinate that is part of
+ * this rectangle, with greater integer X coordinates being part of this
+ * rectangle up to but excluding the right boundary.
+ *
+ * This value MUST be less than or equal to the right boundary. If this
+ * value is equal to the right boundary, the rectangle is empty (has no
+ * width).
+ */
+int left;
+
+/**
+ * The Y coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer Y coordinate that is part of
+ * this rectangle, with greater integer Y coordinates being part of this
+ * rectangle up to but excluding the bottom boundary.
+ *
+ * This value MUST be less than or equal to the bottom boundary.
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2380378676 Aside from a couple of minor typos in the comments, this looks good to 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779349942 ## src/libguac/display-render-thread.c: ## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "display-priv.h" +#include "guacamole/client.h" +#include "guacamole/display.h" +#include "guacamole/flag.h" +#include "guacamole/mem.h" +#include "guacamole/timestamp.h" + +/** + * The maximum duration of a frame in milliseconds. This ensures we at least + * meet a reasonable minimum framerate in the case that the RDP server provides Review Comment: There's a few mentions of an "RDP server" in this file. Copypasta, I assume? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779348901
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,271 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of the given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of the given rectangle
+ * should be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of the given rectangle within the given immutable
+ * (const) buffer, where the upper-left corner of the given buffer is (0, 0).
+ * If the memory address cannot be calculated because doing so would overflow
+ * the maximum value of a size_t, execution of the current process is
+ * automatically aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The const buffer within which the address of the given rectangle should
+ * be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_CONST_BUFFER(rect, buffer, stride, bpp) ((const void*) (\
+((const unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+struct guac_rect {
+
+/**
+ * The X coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer X coordinate that is part of
+ * this rectangle, with greater integer X coordinates being part of this
+ * rectangle up to but excluding the right boundary.
+ *
+ * This value MUST be less than or equal to the right boundary. If this
+ * value is equal to the right boundary, the rectangle is empty (has no
+ * width).
+ */
+int left;
+
+/**
+ * The Y coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer Y coordinate that is part of
+ * this rectangle, with greater integer Y coordinates being part of this
+ * rectangle up to but excluding the bottom boundary.
+ *
+ * This value MUST be less than or equal to the bottom boundary. I
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2380371628 @jmuehlner The "easy mode" render loop is ready, working, and seems substantially better than the previous render loops. I've updated both VNC and RDP accordingly. If anyone needs something more complex than the provided loop, they can just invoke `guac_display_end_multiple_frames()` on their own as they see fit. @necouchman As of right this moment, I think I've resolved all comments. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779347981
##
src/libguac/client.c:
##
@@ -132,7 +132,8 @@ guac_stream* guac_client_alloc_stream(guac_client* client) {
return NULL;
/* Allocate stream */
-stream_index = guac_pool_next_int(client->__stream_pool);
+stream_index = guac_pool_next_int_below_or_die(client->__stream_pool,
Review Comment:
Both should now be fixed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779347939
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of the given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of the given rectangle
+ * should be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of the given rectangle within the given immutable
+ * (const) buffer, where the upper-left corner of the given buffer is (0, 0).
+ * If the memory address cannot be calculated because doing so would overflow
+ * the maximum value of a size_t, execution of the current process is
+ * automatically aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The const buffer within which the address of the given rectangle should
+ * be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_CONST_BUFFER(rect, buffer, stride, bpp) ((const void*) (\
+((const unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+struct guac_rect {
+
+/**
+ * The X coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer X coordinate that is part of
+ * this rectangle, with greater integer X coordinates being part of this
+ * rectangle up to but excluding the right boundary.
+ *
+ * This value MUST be less than or equal to the right boundary. If this
+ * value is equal to the right boundary, the rectangle is empty (has no
+ * width).
+ */
+int left;
+
+/**
+ * The Y coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer Y coordinate that is part of
+ * this rectangle, with greater integer Y coordinates being part of this
+ * rectangle up to but excluding the bottom boundary.
+ *
+ * This value MUST be less than or equal to the bottom boundary.
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1779340206
##
src/libguac/client.c:
##
@@ -132,7 +132,8 @@ guac_stream* guac_client_alloc_stream(guac_client* client) {
return NULL;
/* Allocate stream */
-stream_index = guac_pool_next_int(client->__stream_pool);
+stream_index = guac_pool_next_int_below_or_die(client->__stream_pool,
Review Comment:
> Right above this we do a non-threadsafe check against
GUAC_CLIENT_MAX_STREAMS, which will cause the function to return NULL - do we
still need that? Does the new check not correctly handle this?
Yes - very good catch. I'll remove the superfluous check.
> And if the documented behavior of this function is to return NULL when out
of streams, why are we dying here instead?
Also fair - will fix.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2380357336 > I haven't had any problems with RDP/VNC. In terminal, cat a large file is very slow and can in some cases disconnect me from guacamole and the display is not restored when exiting the alternate buffer. @corentin-soriano Thanks for checking this. The remaining performance trouble with the terminal seems rooted in GUACAMOLE-1256, so if there isn't an obvious fix, I think we'll just end up rolling back those changes and revisiting next release. The changes here should no longer affect that. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2379984401 @necouchman I'll take a look and make sure I've not missed anything. @jmuehlner I just wrote a threaded display render loop that looks like it may improve things yet further for RDP. If that checks out, I'll push it in a moment. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
necouchman commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2379724133 > I've fully reviewed this and don't see any issues with the code as is - does anyone object if I merge this? @mike-jumper did you have any more that you wanted to add? There are a couple of unresolved comments above - one about a missing document block for a function, and one about a non-thread-safe check. Have these been resolved, even though Github is not marking them as outdated? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2379694277 I've fully reviewed this and don't see any issues with the code as is - does anyone object if I merge this? @mike-jumper did you have any more that you wanted to add? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1778884987
##
src/terminal/terminal-handlers.c:
##
@@ -918,31 +1012,87 @@ int guac_terminal_csi(guac_terminal* term, unsigned char
c) {
break;
-/* h: Set Mode */
+/* h: Set Mode (DECSET) */
case 'h':
-
-/* Look up flag and set */
+
+/* Save cursor for later restoration */
+if (argv[0] == GUAC_TERMINAL_DECSET_SAVE_CURSOR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+term->saved_cursor_row = term->cursor_row;
+term->saved_cursor_col = term->cursor_col;
+}
+
+if (term->current_buffer != term->alternate_buffer) {
+
+/* Switch to alternate buffer */
+if (argv[0] == GUAC_TERMINAL_DECSET_USE_ALT_BUFFER
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+
+term->current_buffer = term->alternate_buffer;
+
+/* Update scrollbar bounds (the different buffers have
differing levels of scrollback) */
+guac_terminal_scrollbar_set_bounds(term->scrollbar,
+-guac_terminal_get_available_scroll(term), 0);
+
+}
+
+/* Clear alternate buffer only if we were previously using
+ * the normal buffer */
+if (argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+guac_terminal_clear_range(term,
+0, 0, term->term_height - 1, term->term_width
- 1);
+}
+
+}
+
+/* Look up flag and set */
flag = __guac_terminal_get_flag(term, argv[0],
private_mode_character);
if (flag != NULL)
*flag = true;
-/* Open alternate screen buffer */
-if (argv[0] == GUAC_TERMINAL_ALT_BUFFER)
-guac_terminal_switch_buffers(term, true);
-
break;
-/* l: Reset Mode */
+/* l: Reset Mode (DECRST) */
case 'l':
-
-/* Look up flag and clear */
+
+if (term->current_buffer != term->normal_buffer) {
+
+/* Switch back to normal buffer */
+if (argv[0] == GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER
+|| argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_RESTORE_CURSOR) {
+
+term->current_buffer = term->normal_buffer;
+
+/* Update scrollbar bounds (the different buffers have
differing levels of scrollback) */
+guac_terminal_scrollbar_set_bounds(term->scrollbar,
+-guac_terminal_get_available_scroll(term), 0);
+
+}
+
+/* Clear normal buffer only if we were previously using
+ * the alternate buffer */
+if (argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_CLEAR) {
+guac_terminal_clear_range(term,
+0, 0, term->term_height - 1, term->term_width
- 1);
+}
Review Comment:
> I think we should check if we receive the code
`GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_RESTORE_CURSOR`.
We typically only receive that if `TERM` is set to `xterm-256color` within
the SSH session.
> However guac_terminal_clear_range completely clears the screen instead of
restoring the contents of the normal buffer so we have a black screen with only
the cursor row shown
I believe that's required behavior for this particular DECRST code (1047),
which is defined as clearing the normal buffer after switching to it. From
https://www.xfree86.org/current/ctlseqs.html:
> P s = 1 0 4 7 → Use Normal Screen Buffer, clearing screen first if in the
Alternate Screen (unless disabled by the titeInhibit resource)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
corentin-soriano commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1778574792
##
src/terminal/terminal-handlers.c:
##
@@ -918,31 +1012,87 @@ int guac_terminal_csi(guac_terminal* term, unsigned char
c) {
break;
-/* h: Set Mode */
+/* h: Set Mode (DECSET) */
case 'h':
-
-/* Look up flag and set */
+
+/* Save cursor for later restoration */
+if (argv[0] == GUAC_TERMINAL_DECSET_SAVE_CURSOR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+term->saved_cursor_row = term->cursor_row;
+term->saved_cursor_col = term->cursor_col;
+}
+
+if (term->current_buffer != term->alternate_buffer) {
+
+/* Switch to alternate buffer */
+if (argv[0] == GUAC_TERMINAL_DECSET_USE_ALT_BUFFER
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+
+term->current_buffer = term->alternate_buffer;
+
+/* Update scrollbar bounds (the different buffers have
differing levels of scrollback) */
+guac_terminal_scrollbar_set_bounds(term->scrollbar,
+-guac_terminal_get_available_scroll(term), 0);
+
+}
+
+/* Clear alternate buffer only if we were previously using
+ * the normal buffer */
+if (argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECSET_USE_ALT_BUFFER_AND_SAVE_CURSOR_AND_CLEAR) {
+guac_terminal_clear_range(term,
+0, 0, term->term_height - 1, term->term_width
- 1);
+}
+
+}
+
+/* Look up flag and set */
flag = __guac_terminal_get_flag(term, argv[0],
private_mode_character);
if (flag != NULL)
*flag = true;
-/* Open alternate screen buffer */
-if (argv[0] == GUAC_TERMINAL_ALT_BUFFER)
-guac_terminal_switch_buffers(term, true);
-
break;
-/* l: Reset Mode */
+/* l: Reset Mode (DECRST) */
case 'l':
-
-/* Look up flag and clear */
+
+if (term->current_buffer != term->normal_buffer) {
+
+/* Switch back to normal buffer */
+if (argv[0] == GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER
+|| argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_CLEAR
+|| argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_RESTORE_CURSOR) {
+
+term->current_buffer = term->normal_buffer;
+
+/* Update scrollbar bounds (the different buffers have
differing levels of scrollback) */
+guac_terminal_scrollbar_set_bounds(term->scrollbar,
+-guac_terminal_get_available_scroll(term), 0);
+
+}
+
+/* Clear normal buffer only if we were previously using
+ * the alternate buffer */
+if (argv[0] ==
GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_CLEAR) {
+guac_terminal_clear_range(term,
+0, 0, term->term_height - 1, term->term_width
- 1);
+}
Review Comment:
Actually, when closing vi (or other), only the line with the cursor is
updated and the text file content stay on screen.
I think we should check if we receive the code
`GUAC_TERMINAL_DECRST_USE_NORMAL_BUFFER_AND_RESTORE_CURSOR`.
However `guac_terminal_clear_range` completely clears the screen instead of
restoring the contents of the normal buffer so we have a black screen with only
the cursor row shown.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1777850195
##
src/libguac/client.c:
##
@@ -132,7 +132,8 @@ guac_stream* guac_client_alloc_stream(guac_client* client) {
return NULL;
/* Allocate stream */
-stream_index = guac_pool_next_int(client->__stream_pool);
+stream_index = guac_pool_next_int_below_or_die(client->__stream_pool,
Review Comment:
Right above this we do a non-threadsafe check against
`GUAC_CLIENT_MAX_STREAMS`, which will cause the function to return `NULL` - do
we still need that? Does the new check not correctly handle this?
And if the documented behavior of this function is to return `NULL` when out
of streams, why are we dying here instead?
It seems like we'd want to maintain that `NULL` return for backwards
compatibility, but also I'd point out that none of the callers of these
`alloc_stream()` functions look to actually check if the return value is
non-NULL before using it...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2353455294 > > In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.  > > This should now be resolved - it was caused by the `GotCopyRect()` function having been overridden. Previously, this was no issue because we used our own buffer. Now that we're reusing libvncclient's buffer, the original `GotCopyRect()` needs to be called or that buffer won't be updated with respect to received `CopyRect` updates. Nice - I can confirm that this issue looks to be fixed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2351110467 > In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.  This should now be resolved - it was caused by the `GotCopyRect()` function having been overridden. Previously, this was no issue because we used our own buffer. Now that we're reusing libvncclient's buffer, the original `GotCopyRect()` needs to be called or that buffer won't be updated with respect to received `CopyRect` updates. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1757704771
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of the given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of the given rectangle
+ * should be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of the given rectangle within the given immutable
+ * (const) buffer, where the upper-left corner of the given buffer is (0, 0).
+ * If the memory address cannot be calculated because doing so would overflow
+ * the maximum value of a size_t, execution of the current process is
+ * automatically aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The const buffer within which the address of the given rectangle should
+ * be determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of the given rectangle within the given buffer.
+ */
+#define GUAC_RECT_CONST_BUFFER(rect, buffer, stride, bpp) ((const void*) (\
+((const unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+struct guac_rect {
+
+/**
+ * The X coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer X coordinate that is part of
+ * this rectangle, with greater integer X coordinates being part of this
+ * rectangle up to but excluding the right boundary.
+ *
+ * This value MUST be less than or equal to the right boundary. If this
+ * value is equal to the right boundary, the rectangle is empty (has no
+ * width).
+ */
+int left;
+
+/**
+ * The Y coordinate of the upper-left corner of this rectangle (inclusive).
+ * This value represents the least integer Y coordinate that is part of
+ * this rectangle, with greater integer Y coordinates being part of this
+ * rectangle up to but excluding the bottom boundary.
+ *
+ * This value MUST be less than or equal to the bottom boundary. I
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2344328359 In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.  -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2342842845 OK - I think I have a handle on the performance regression. The issue seems to have been mainly a memory bottleneck, with the solution being: 1. Use `memcpy()` where possible 2. Provide to mechanism to point a `guac_display_layer` at an external buffer (to avoid having to copy from the framebuffer of a client library) I'm putting the above in place, plus a partial revert of the terminal emulator changes (migrating the display back to the character-based display). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2327687976 Ok, read through everything, and it all looks reasonable except for a few minor formatting / wording issues. I'm looking forward to using this! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742842853
##
src/terminal/terminal/terminal-priv.h:
##
@@ -325,23 +290,31 @@ struct guac_terminal {
guac_terminal_display* display;
/**
- * Current terminal display state. All characters present on the screen
- * are within this buffer. This has nothing to do with the display, which
- * facilitates transfer of a set of changes to the remote display.
+ * The default, "normal" buffer containing all characters that should be
+ * displayed within the terminal emulator while not using the alternate
+ * buffer. Unless switched to the alternate buffer, all terminal operations
+ * will involve this buffer. The buffer that is relevant to terminal
+ * operations is determined by the current value of current_buffer.
*/
-guac_terminal_buffer* buffer;
+guac_terminal_buffer* normal_buffer;
/**
- * Alternate buffer.
+ * The non-default, "alternate" buffer containing all characters that
should
+ * be displayed within the terminal emulator while not using the normal
+ * buffer. Unless switched to the alternate buffer, all terminal operations
Review Comment:
Copypasta? "Unless switched to the alternate buffer, all terminal operations
will involve this buffer"/
##
src/terminal/terminal/terminal-priv.h:
##
@@ -325,23 +290,31 @@ struct guac_terminal {
guac_terminal_display* display;
/**
- * Current terminal display state. All characters present on the screen
- * are within this buffer. This has nothing to do with the display, which
- * facilitates transfer of a set of changes to the remote display.
+ * The default, "normal" buffer containing all characters that should be
+ * displayed within the terminal emulator while not using the alternate
+ * buffer. Unless switched to the alternate buffer, all terminal operations
+ * will involve this buffer. The buffer that is relevant to terminal
+ * operations is determined by the current value of current_buffer.
*/
-guac_terminal_buffer* buffer;
+guac_terminal_buffer* normal_buffer;
/**
- * Alternate buffer.
+ * The non-default, "alternate" buffer containing all characters that
should
+ * be displayed within the terminal emulator while not using the normal
+ * buffer. Unless switched to the alternate buffer, all terminal operations
Review Comment:
Copypasta? "Unless switched to the alternate buffer, all terminal operations
will involve this buffer".
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742842378
##
src/terminal/terminal/terminal-priv.h:
##
@@ -77,32 +91,20 @@ struct guac_terminal {
* a download of a given remote file.
*/
guac_terminal_file_download_handler* file_download_handler;
-
+
Review Comment:
It doesn't look like this new whitespace belongs 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742773683
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of given rectangle should be
+ * determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of given rectangle within the given immutable
Review Comment:
Fixed via rebase.
##
src/terminal/buffer.c:
##
@@ -19,13 +19,85 @@
#include "terminal/buffer.h"
#include "terminal/common.h"
+#include "terminal/terminal.h"
+#include
#include
+#include
#include
#include
-guac_terminal_buffer* guac_terminal_buffer_alloc(int rows, guac_terminal_char*
default_character) {
+#define GUAC_TERMINAL_BUFFER_ROW_MIN_SIZE 256
Review Comment:
Fixed via rebase.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742771893 ## src/libguac/guacamole/rect.h: ## @@ -0,0 +1,256 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#ifndef GUAC_RECT_H +#define GUAC_RECT_H + +#include "mem.h" +#include "rect-types.h" + +/** + * Returns the memory address of given rectangle within the given mutable + * buffer, where the upper-left corner of the given buffer is (0, 0). If the + * memory address cannot be calculated because doing so would overflow the + * maximum value of a size_t, execution of the current process is automatically + * aborted. + * + * IMPORTANT: No checks are performed on whether the rectangle extends beyond + * the bounds of the buffer, including considering whether the left/top + * position of the rectangle is negative. If the rectangle has not already been + * contrained to be within the bounds of the buffer, such checks must be + * performed before dereferencing the value returned by this macro. + * + * @param rect + * The rectangle to determine the offset of. + * + * @param buffer + * The mutable buffer within which the address of given rectangle should be + * determined. + * + * @param stride + * The number of bytes in each row of image data within the buffer. + * + * @param bpp + * The number of bytes in each pixel of image data. + * + * @return + * The memory address of given rectangle within the given buffer. + */ +#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\ +((unsigned char*) (buffer)) \ ++ guac_mem_ckd_mul_or_die((rect).top, stride) \ ++ guac_mem_ckd_mul_or_die((rect).left, bpp))) + +/** + * Returns the memory address of given rectangle within the given immutable Review Comment: Ooof, I say "of given rectangle" nearly everywhere... I'll fix all the occurrences. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742767373
##
src/libguac/guacamole/rect.h:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RECT_H
+#define GUAC_RECT_H
+
+#include "mem.h"
+#include "rect-types.h"
+
+/**
+ * Returns the memory address of given rectangle within the given mutable
+ * buffer, where the upper-left corner of the given buffer is (0, 0). If the
+ * memory address cannot be calculated because doing so would overflow the
+ * maximum value of a size_t, execution of the current process is automatically
+ * aborted.
+ *
+ * IMPORTANT: No checks are performed on whether the rectangle extends beyond
+ * the bounds of the buffer, including considering whether the left/top
+ * position of the rectangle is negative. If the rectangle has not already been
+ * contrained to be within the bounds of the buffer, such checks must be
+ * performed before dereferencing the value returned by this macro.
+ *
+ * @param rect
+ * The rectangle to determine the offset of.
+ *
+ * @param buffer
+ * The mutable buffer within which the address of given rectangle should be
+ * determined.
+ *
+ * @param stride
+ * The number of bytes in each row of image data within the buffer.
+ *
+ * @param bpp
+ * The number of bytes in each pixel of image data.
+ *
+ * @return
+ * The memory address of given rectangle within the given buffer.
+ */
+#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\
+((unsigned char*) (buffer)) \
++ guac_mem_ckd_mul_or_die((rect).top, stride) \
++ guac_mem_ckd_mul_or_die((rect).left, bpp)))
+
+/**
+ * Returns the memory address of given rectangle within the given immutable
Review Comment:
Yep, I'll fix that.
##
src/terminal/buffer.c:
##
@@ -19,13 +19,85 @@
#include "terminal/buffer.h"
#include "terminal/common.h"
+#include "terminal/terminal.h"
+#include
#include
+#include
#include
#include
-guac_terminal_buffer* guac_terminal_buffer_alloc(int rows, guac_terminal_char*
default_character) {
+#define GUAC_TERMINAL_BUFFER_ROW_MIN_SIZE 256
Review Comment:
Oopsies.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742698522
##
src/terminal/buffer.c:
##
@@ -19,13 +19,85 @@
#include "terminal/buffer.h"
#include "terminal/common.h"
+#include "terminal/terminal.h"
+#include
#include
+#include
#include
#include
-guac_terminal_buffer* guac_terminal_buffer_alloc(int rows, guac_terminal_char*
default_character) {
+#define GUAC_TERMINAL_BUFFER_ROW_MIN_SIZE 256
Review Comment:
This constant should be documented.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742683237 ## src/libguac/guacamole/rect.h: ## @@ -0,0 +1,256 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#ifndef GUAC_RECT_H +#define GUAC_RECT_H + +#include "mem.h" +#include "rect-types.h" + +/** + * Returns the memory address of given rectangle within the given mutable + * buffer, where the upper-left corner of the given buffer is (0, 0). If the + * memory address cannot be calculated because doing so would overflow the + * maximum value of a size_t, execution of the current process is automatically + * aborted. + * + * IMPORTANT: No checks are performed on whether the rectangle extends beyond + * the bounds of the buffer, including considering whether the left/top + * position of the rectangle is negative. If the rectangle has not already been + * contrained to be within the bounds of the buffer, such checks must be + * performed before dereferencing the value returned by this macro. + * + * @param rect + * The rectangle to determine the offset of. + * + * @param buffer + * The mutable buffer within which the address of given rectangle should be + * determined. + * + * @param stride + * The number of bytes in each row of image data within the buffer. + * + * @param bpp + * The number of bytes in each pixel of image data. + * + * @return + * The memory address of given rectangle within the given buffer. + */ +#define GUAC_RECT_MUTABLE_BUFFER(rect, buffer, stride, bpp) ((void*) (\ +((unsigned char*) (buffer)) \ ++ guac_mem_ckd_mul_or_die((rect).top, stride) \ ++ guac_mem_ckd_mul_or_die((rect).left, bpp))) + +/** + * Returns the memory address of given rectangle within the given immutable Review Comment: Should this be "a given rectangle" or "the given rectangle"? Reads a bit odd as is. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742498031 ## src/common-ssh/key.c: ## @@ -132,18 +132,25 @@ guac_common_ssh_key* guac_common_ssh_key_alloc(char* data, int length, * different key algorithms) we need to perform a heuristic here to check * if a passphrase is needed. This could allow junk keys through that * would never be able to auth. libssh2 should display errors to help - * admins track down malformed keys and delete or replace them. - */ + * admins track down malformed keys and delete or replace them. */ if (is_passphrase_needed(data, length) && (passphrase == NULL || *passphrase == '\0')) return NULL; guac_common_ssh_key* key = guac_mem_alloc(sizeof(guac_common_ssh_key)); +/* NOTE: Older versions of libssh2 will at times ignore the declared key + * length and instead recalculate the length using strlen(). This has since + * been fixed, but as of this writing the fix has not yet been released. + * Below, we add our own null terminator to ensure that such calls to + * strlen() will work without issue. We can remove this workaround once + * copies of libssh2 that use strlen() on key data are not in common use. */ + /* Copy private key to structure */ key->private_key_length = length; -key->private_key = guac_mem_alloc(length); +key->private_key = guac_mem_alloc(guac_mem_ckd_add_or_die(length, 1)); /* Extra byte added here for null terminator (see above) */ memcpy(key->private_key, data, length); Review Comment: Here we're copying binary data, not a string. The null terminator is added purely because libssh2 may incorrectly call `strlen()` on that data even though it's not a string. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1742480638 ## src/common-ssh/key.c: ## @@ -132,18 +132,25 @@ guac_common_ssh_key* guac_common_ssh_key_alloc(char* data, int length, * different key algorithms) we need to perform a heuristic here to check * if a passphrase is needed. This could allow junk keys through that * would never be able to auth. libssh2 should display errors to help - * admins track down malformed keys and delete or replace them. - */ + * admins track down malformed keys and delete or replace them. */ if (is_passphrase_needed(data, length) && (passphrase == NULL || *passphrase == '\0')) return NULL; guac_common_ssh_key* key = guac_mem_alloc(sizeof(guac_common_ssh_key)); +/* NOTE: Older versions of libssh2 will at times ignore the declared key + * length and instead recalculate the length using strlen(). This has since + * been fixed, but as of this writing the fix has not yet been released. + * Below, we add our own null terminator to ensure that such calls to + * strlen() will work without issue. We can remove this workaround once + * copies of libssh2 that use strlen() on key data are not in common use. */ + /* Copy private key to structure */ key->private_key_length = length; -key->private_key = guac_mem_alloc(length); +key->private_key = guac_mem_alloc(guac_mem_ckd_add_or_die(length, 1)); /* Extra byte added here for null terminator (see above) */ memcpy(key->private_key, data, length); Review Comment: Why not use `guac_strlcpy()` 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
necouchman commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2323632631 > **_FINALLY_** ready for review. 😌 Whew, just almost 10K lines of code to review. Shouldn't take more than a few minutes ;-). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2323615317 **_FINALLY_** ready for review. :relieved: -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1739937324
##
src/protocols/rdp/rdp.c:
##
@@ -143,6 +142,7 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) {
if (!gdi_init(instance, guac_rdp_get_native_pixel_format(FALSE)))
return FALSE;
+#if 0
Review Comment:
RDP and VNC should now be complete and no longer contain any of these `#if
0` blocks.
I'm now just finishing up the last terminal emulator bits and retesting.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1737075377
##
src/protocols/rdp/bitmap.c:
##
@@ -79,54 +82,53 @@ BOOL guac_rdp_bitmap_paint(rdpContext* context, rdpBitmap*
bitmap) {
guac_client* client = ((rdp_freerdp_context*) context)->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
-guac_common_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
-
-int width = bitmap->right - bitmap->left + 1;
-int height = bitmap->bottom - bitmap->top + 1;
+guac_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
/* If not cached, cache if necessary */
if (buffer == NULL && ((guac_rdp_bitmap*) bitmap)->used >= 1)
guac_rdp_cache_bitmap(context, bitmap);
-/* If cached, retrieve from cache */
-if (buffer != NULL)
-guac_common_surface_copy(buffer->surface, 0, 0, width, height,
-rdp_client->display->default_surface,
-bitmap->left, bitmap->top);
-
-/* Otherwise, draw with stored image data */
-else if (bitmap->data != NULL) {
+guac_display_layer* default_layer =
guac_display_default_layer(rdp_client->display);
+guac_display_layer_raw_context* dst_context =
guac_display_layer_open_raw(default_layer);
-/* Create surface from image data */
-cairo_surface_t* image = cairo_image_surface_create_for_data(
-bitmap->data, CAIRO_FORMAT_RGB24,
-width, height, 4*bitmap->width);
+guac_rect dst_rect = {
+.left = bitmap->left,
+.top= bitmap->top,
+.right = bitmap->right,
+.bottom = bitmap->bottom
+};
-/* Draw image on default surface */
-guac_common_surface_draw(rdp_client->display->default_surface,
-bitmap->left, bitmap->top, image);
+guac_rect_constrain(&dst_rect, &dst_context->bounds);
-/* Free surface */
-cairo_surface_destroy(image);
+/* If cached, retrieve from cache */
+if (buffer != NULL) {
+guac_display_layer_raw_context* src_context =
guac_display_layer_open_raw(buffer);
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
src_context->buffer, src_context->stride);
+}
+/* Otherwise, draw with stored image data */
+else if (bitmap->data != NULL) {
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
bitmap->data, 4 * bitmap->width);
Review Comment:
Probably worth commenting on
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1735401269
##
src/protocols/rdp/bitmap.c:
##
@@ -79,54 +82,53 @@ BOOL guac_rdp_bitmap_paint(rdpContext* context, rdpBitmap*
bitmap) {
guac_client* client = ((rdp_freerdp_context*) context)->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
-guac_common_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
-
-int width = bitmap->right - bitmap->left + 1;
-int height = bitmap->bottom - bitmap->top + 1;
+guac_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
/* If not cached, cache if necessary */
if (buffer == NULL && ((guac_rdp_bitmap*) bitmap)->used >= 1)
guac_rdp_cache_bitmap(context, bitmap);
-/* If cached, retrieve from cache */
-if (buffer != NULL)
-guac_common_surface_copy(buffer->surface, 0, 0, width, height,
-rdp_client->display->default_surface,
-bitmap->left, bitmap->top);
-
-/* Otherwise, draw with stored image data */
-else if (bitmap->data != NULL) {
+guac_display_layer* default_layer =
guac_display_default_layer(rdp_client->display);
+guac_display_layer_raw_context* dst_context =
guac_display_layer_open_raw(default_layer);
-/* Create surface from image data */
-cairo_surface_t* image = cairo_image_surface_create_for_data(
-bitmap->data, CAIRO_FORMAT_RGB24,
-width, height, 4*bitmap->width);
+guac_rect dst_rect = {
+.left = bitmap->left,
+.top= bitmap->top,
+.right = bitmap->right,
+.bottom = bitmap->bottom
+};
-/* Draw image on default surface */
-guac_common_surface_draw(rdp_client->display->default_surface,
-bitmap->left, bitmap->top, image);
+guac_rect_constrain(&dst_rect, &dst_context->bounds);
-/* Free surface */
-cairo_surface_destroy(image);
+/* If cached, retrieve from cache */
+if (buffer != NULL) {
+guac_display_layer_raw_context* src_context =
guac_display_layer_open_raw(buffer);
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
src_context->buffer, src_context->stride);
+}
+/* Otherwise, draw with stored image data */
+else if (bitmap->data != NULL) {
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
bitmap->data, 4 * bitmap->width);
Review Comment:
Each pixel within the image data at `bitmap->data` is 32-bit.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1735383330
##
src/protocols/rdp/rdp.c:
##
@@ -143,6 +142,7 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) {
if (!gdi_init(instance, guac_rdp_get_native_pixel_format(FALSE)))
return FALSE;
+#if 0
Review Comment:
Looks like I should wait for this stuff to be finalized before reviewing the
protocols
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1735375622
##
src/protocols/rdp/bitmap.c:
##
@@ -79,54 +82,53 @@ BOOL guac_rdp_bitmap_paint(rdpContext* context, rdpBitmap*
bitmap) {
guac_client* client = ((rdp_freerdp_context*) context)->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
-guac_common_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
-
-int width = bitmap->right - bitmap->left + 1;
-int height = bitmap->bottom - bitmap->top + 1;
+guac_display_layer* buffer = ((guac_rdp_bitmap*) bitmap)->layer;
/* If not cached, cache if necessary */
if (buffer == NULL && ((guac_rdp_bitmap*) bitmap)->used >= 1)
guac_rdp_cache_bitmap(context, bitmap);
-/* If cached, retrieve from cache */
-if (buffer != NULL)
-guac_common_surface_copy(buffer->surface, 0, 0, width, height,
-rdp_client->display->default_surface,
-bitmap->left, bitmap->top);
-
-/* Otherwise, draw with stored image data */
-else if (bitmap->data != NULL) {
+guac_display_layer* default_layer =
guac_display_default_layer(rdp_client->display);
+guac_display_layer_raw_context* dst_context =
guac_display_layer_open_raw(default_layer);
-/* Create surface from image data */
-cairo_surface_t* image = cairo_image_surface_create_for_data(
-bitmap->data, CAIRO_FORMAT_RGB24,
-width, height, 4*bitmap->width);
+guac_rect dst_rect = {
+.left = bitmap->left,
+.top= bitmap->top,
+.right = bitmap->right,
+.bottom = bitmap->bottom
+};
-/* Draw image on default surface */
-guac_common_surface_draw(rdp_client->display->default_surface,
-bitmap->left, bitmap->top, image);
+guac_rect_constrain(&dst_rect, &dst_context->bounds);
-/* Free surface */
-cairo_surface_destroy(image);
+/* If cached, retrieve from cache */
+if (buffer != NULL) {
+guac_display_layer_raw_context* src_context =
guac_display_layer_open_raw(buffer);
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
src_context->buffer, src_context->stride);
+}
+/* Otherwise, draw with stored image data */
+else if (bitmap->data != NULL) {
+guac_display_layer_raw_context_put(dst_context, &dst_rect,
bitmap->data, 4 * bitmap->width);
Review Comment:
Why 4?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1735366761
##
src/libguac/tests/rect/intersects.c:
##
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include
+#include
+
+/**
+ * Test which verifies intersection testing via guac_rect_intersects().
+ */
+void test_rect__intersects() {
+
+int res;
+
+guac_rect min;
+guac_rect rect;
+
+guac_rect_init(&min, 10, 10, 10, 10);
+
+/* Rectangle intersection - empty
+ * rectangle is outside */
+guac_rect_init(&rect, 25, 25, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_FALSE(res);
+
+/* Rectangle intersection - empty
+ * rectangle is outside */
+guac_rect_init(&rect, 20, 20, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_FALSE(res);
+
+/* Rectangle intersection - complete
+ * rectangle is completely inside */
+guac_rect_init(&rect, 11, 11, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_TRUE(res);
+
+/* Rectangle intersection - partial
+ * rectangle intersects UL */
+guac_rect_init(&rect, 8, 8, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_TRUE(res);
+
+/* Rectangle intersection - partial
+ * rectangle intersects LR */
+guac_rect_init(&rect, 18, 18, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_TRUE(res);
+
+/* Rectangle intersection - complete
+ * rect intersects along UL but inside */
+guac_rect_init(&rect, 10, 10, 5, 5);
+res = guac_rect_intersects(&rect, &min);
+CU_ASSERT_TRUE(res);
+
+/* Rectangle intersection - partial
Review Comment:
This terminoloy here about a partial rectangle intersection is sort of
confusing - it looks like these rectangles are _touching_ at an edge, but not
actually overlapping, right?
Maybe consider rewording to make it clearer what these comments mean.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1735363102
##
src/libguac/tests/rect/align.c:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include
+#include
+
+/**
+ * Test which verifies guac_rect_align() properly shifts and resizes rectangles
+ * to fit an NxN grid.
+ */
+void test_rect__align() {
+
+int cell_size = 4;
+
+guac_rect rect;
+
+/* Simple adjustment */
+guac_rect_init(&rect, 0, 0, 25, 25);
+guac_rect_align(&rect, cell_size);
+CU_ASSERT_EQUAL(0, rect.left);
+CU_ASSERT_EQUAL(0, rect.top);
+CU_ASSERT_EQUAL(32, rect.right);
+CU_ASSERT_EQUAL(32, rect.bottom);
+
+/* Adjustment with moving of rect */
+guac_rect_init(&rect, 75, 75, 25, 25);
+guac_rect_align(&rect, cell_size);
+CU_ASSERT_EQUAL(64, rect.left);
+CU_ASSERT_EQUAL(64, rect.top);
+CU_ASSERT_EQUAL(112, rect.right);
+CU_ASSERT_EQUAL(112, rect.bottom);
+
+guac_rect_init(&rect, -5, -5, 25, 25);
Review Comment:
Probably worth mentioning what this test is doing as well.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733578980
##
src/libguac/display-cursor.c:
##
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "display-builtin-cursors.h"
+#include "display-priv.h"
+#include "guacamole/display.h"
+#include "guacamole/mem.h"
+#include "guacamole/rect.h"
+#include "guacamole/rwlock.h"
+
+#include
+
+guac_display_layer* guac_display_cursor(guac_display* display) {
+return display->cursor_buffer;
+}
+
+void guac_display_set_cursor_hotspot(guac_display* display, int x, int y) {
+guac_rwlock_acquire_write_lock(&display->pending_frame.lock);
+
+display->pending_frame.cursor_hotspot_x = x;
+display->pending_frame.cursor_hotspot_y = y;
+
+guac_rwlock_release_lock(&display->pending_frame.lock);
+}
+
+void guac_display_set_cursor(guac_display* display,
+guac_display_cursor_type cursor_type) {
+
+/* Translate requested type into built-in cursor */
+const guac_display_builtin_cursor* cursor;
+switch (cursor_type) {
+
+case GUAC_DISPLAY_CURSOR_NONE:
+cursor = &guac_display_cursor_none;
+break;
+
+case GUAC_DISPLAY_CURSOR_DOT:
+cursor = &guac_display_cursor_dot;
+break;
+
+case GUAC_DISPLAY_CURSOR_IBAR:
+cursor = &guac_display_cursor_ibar;
+break;
+
+case GUAC_DISPLAY_CURSOR_POINTER:
+default:
+cursor = &guac_display_cursor_pointer;
+break;
+
+}
+
+/* Resize cursor to fit requested icon */
+guac_display_layer* cursor_layer = guac_display_cursor(display);
+guac_display_layer_resize(cursor_layer, cursor->width, cursor->height);
+
+/* Copy over graphical content of cursor icon ... */
+
Review Comment:
Based on what I'm seeing elsewhere, I'm guessing yes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733568955
##
src/libguac/display-cursor.c:
##
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "display-builtin-cursors.h"
+#include "display-priv.h"
+#include "guacamole/display.h"
+#include "guacamole/mem.h"
+#include "guacamole/rect.h"
+#include "guacamole/rwlock.h"
+
+#include
+
+guac_display_layer* guac_display_cursor(guac_display* display) {
+return display->cursor_buffer;
+}
+
+void guac_display_set_cursor_hotspot(guac_display* display, int x, int y) {
+guac_rwlock_acquire_write_lock(&display->pending_frame.lock);
+
+display->pending_frame.cursor_hotspot_x = x;
+display->pending_frame.cursor_hotspot_y = y;
+
+guac_rwlock_release_lock(&display->pending_frame.lock);
+}
+
+void guac_display_set_cursor(guac_display* display,
+guac_display_cursor_type cursor_type) {
+
+/* Translate requested type into built-in cursor */
+const guac_display_builtin_cursor* cursor;
+switch (cursor_type) {
+
+case GUAC_DISPLAY_CURSOR_NONE:
+cursor = &guac_display_cursor_none;
+break;
+
+case GUAC_DISPLAY_CURSOR_DOT:
+cursor = &guac_display_cursor_dot;
+break;
+
+case GUAC_DISPLAY_CURSOR_IBAR:
+cursor = &guac_display_cursor_ibar;
+break;
+
+case GUAC_DISPLAY_CURSOR_POINTER:
+default:
+cursor = &guac_display_cursor_pointer;
+break;
+
+}
+
+/* Resize cursor to fit requested icon */
+guac_display_layer* cursor_layer = guac_display_cursor(display);
+guac_display_layer_resize(cursor_layer, cursor->width, cursor->height);
+
+/* Copy over graphical content of cursor icon ... */
+
+guac_display_layer_raw_context* context =
guac_display_layer_open_raw(cursor_layer);
+
+const unsigned char* src_cursor_row = cursor->buffer;
+unsigned char* dst_cursor_row = context->buffer;
+size_t row_length = guac_mem_ckd_mul_or_die(cursor->width, 4);
+
+for (int y = 0; y < cursor->height; y++) {
+memcpy(dst_cursor_row, src_cursor_row, row_length);
+src_cursor_row += cursor->stride;
+dst_cursor_row += context->stride;
+}
+
+/* ... and cursor hotspot */
+guac_display_set_cursor_hotspot(display, cursor->hotspot_x,
cursor->hotspot_y);
+
+/* Update to cursor icon is now complete - notify display */
+
Review Comment:
Or 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525:
URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733567577
##
src/libguac/display-cursor.c:
##
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "display-builtin-cursors.h"
+#include "display-priv.h"
+#include "guacamole/display.h"
+#include "guacamole/mem.h"
+#include "guacamole/rect.h"
+#include "guacamole/rwlock.h"
+
+#include
+
+guac_display_layer* guac_display_cursor(guac_display* display) {
+return display->cursor_buffer;
+}
+
+void guac_display_set_cursor_hotspot(guac_display* display, int x, int y) {
+guac_rwlock_acquire_write_lock(&display->pending_frame.lock);
+
+display->pending_frame.cursor_hotspot_x = x;
+display->pending_frame.cursor_hotspot_y = y;
+
+guac_rwlock_release_lock(&display->pending_frame.lock);
+}
+
+void guac_display_set_cursor(guac_display* display,
+guac_display_cursor_type cursor_type) {
+
+/* Translate requested type into built-in cursor */
+const guac_display_builtin_cursor* cursor;
+switch (cursor_type) {
+
+case GUAC_DISPLAY_CURSOR_NONE:
+cursor = &guac_display_cursor_none;
+break;
+
+case GUAC_DISPLAY_CURSOR_DOT:
+cursor = &guac_display_cursor_dot;
+break;
+
+case GUAC_DISPLAY_CURSOR_IBAR:
+cursor = &guac_display_cursor_ibar;
+break;
+
+case GUAC_DISPLAY_CURSOR_POINTER:
+default:
+cursor = &guac_display_cursor_pointer;
+break;
+
+}
+
+/* Resize cursor to fit requested icon */
+guac_display_layer* cursor_layer = guac_display_cursor(display);
+guac_display_layer_resize(cursor_layer, cursor->width, cursor->height);
+
+/* Copy over graphical content of cursor icon ... */
+
Review Comment:
Did you mean to have a newline 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733566206 ## .gitignore: ## @@ -48,3 +48,7 @@ doc/*/doxygen-output # IDE metadata nbproject/ + +# Compilation database, as may be generated by tools like Bear Review 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733565322 ## .gitignore: ## @@ -48,3 +48,7 @@ doc/*/doxygen-output # IDE metadata nbproject/ + +# Compilation database, as may be generated by tools like Bear Review Comment: Another case where GitHub's selection of only 8 emojis for reactions feels woefully inadequate. :bear: -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
jmuehlner commented on code in PR #525: URL: https://github.com/apache/guacamole-server/pull/525#discussion_r1733562274 ## .gitignore: ## @@ -48,3 +48,7 @@ doc/*/doxygen-output # IDE metadata nbproject/ + +# Compilation database, as may be generated by tools like Bear Review Comment: Huh, had never heard of Bear before. TIL -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering. [guacamole-server]
mike-jumper commented on PR #525: URL: https://github.com/apache/guacamole-server/pull/525#issuecomment-2161613877 I've been using the following patch to observe the impact these changes: https://gist.github.com/mike-jumper/0afd41c9fbcbc764f719890fd0dd4e3b With the above patch in place, you can set `Guacamole.Display.DEBUG` to `true` to enable a debug mode in which draw operations to the Guacamole display are highlighted in colored rectangles, where the color varies by the type of operation: * **RED:** Draw of compressed image data (the most expensive operation) * **BLUE:** Copy of existing image data (ie: an optimized scroll, restoring data from a cache - much cheaper than encoding, sending, and decoding image data) * **GREEN:** Solid-color rectangular draw (a combination of `rect` and `cfill` - very cheap). ### Behavior of `guac_common_surface` Here, everything is red. Dirty rects are generally nicely split and tightened around the regions actually changed, but that's about as deep as the processing goes. Without information from the remote desktop server explicitly stating that a particular update is a copy, we can't detect that a copy would be better. https://github.com/apache/guacamole-server/assets/4632905/b0792ae3-f1a7-4bc6-8ce1-932832c32a21 ### Behavior of `guac_display` Now, things get much more colorful. Things that can be represented more efficiently as copies or rectangles are automatically optimized. This includes cases when the nature of those updates would prevent even the remote desktop server from knowing that a copy has occurred. https://github.com/apache/guacamole-server/assets/4632905/6fb37e05-37c3-4eef-ba42-88f4c89442a7 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
