You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "grintor (via GitHub)" <gi...@apache.org> on 2026/07/06 16:28:33 UTC
[PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
grintor opened a new pull request, #687:
URL: https://github.com/apache/guacamole-server/pull/687
Added a -o option to guacenc allowing the output path to be specified explicitly. It defaults to FILE.m4v as before when unspecified for backwards compatibility. Specifying "-" as the output path streams the encoded video to stdout. Normal MP4 (which is what's being used for file output still) doesn't support streaming, so we use fragmented MP4 here when outputting to stdout. This paves the way to streaming that stdout to a video player or even the browser in real time as it's being encoded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub)" <gi...@apache.org>.
grintor commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-5682690798
Ok this latest commit closes GUACAMOLE-974 in addition to the GUACAMOLE-1370 the original pull request by adding your requested feature. I also noticed that my original pull request closes GUACAMOLE-973 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-4961462963
@grintor You may need to rebase this on top of the current `main` branch - there were some issues with the main branch there for a while, and the build check is currently failing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#discussion_r3537700291
##########
src/guacenc/guacenc.c:
##########
@@ -106,15 +120,27 @@ int main(int argc, char* argv[]) {
/* Get current filename */
const char* path = argv[i];
- /* Generate output filename */
- char out_path[4096];
- int len = snprintf(out_path, sizeof(out_path), "%s.m4v", path);
+ /* Use the explicitly-provided output path, if any, streaming to
+ * STDOUT if "-" was given as the output */
+ const char* out_path;
+ char out_buffer[4096];
+ if (out_file != NULL)
+ out_path = strcmp(out_file, "-") ? out_file : GUACENC_STDOUT_PATH;
+
+ /* Otherwise, generate output filename from input filename */
+ else {
+
+ int len = snprintf(out_buffer, sizeof(out_buffer), "%s.m4v", path);
+
+ /* Do not write if filename exceeds maximum length */
+ if (len >= sizeof(out_buffer)) {
+ guacenc_log(GUAC_LOG_ERROR, "Cannot write output file for "
+ "\"%s\": Name too long", path);
+ continue;
+ }
+
+ out_path = out_buffer;
Review Comment:
What if the filename that the user specifies is too long? It looks like this logic only catches generated filenames that may be too long, and doesn't account for something being given by the user that is too long?
##########
src/guacenc/video.c:
##########
@@ -54,8 +56,16 @@ guacenc_video* guacenc_video_alloc(const char* path, const char* codec_name,
int ret;
int failed_header = 0;
+ /* The container format cannot be guessed from a pipe URL and must be
+ * specified explicitly. The "ipod" container is used, matching the
+ * container that would be guessed from the ".m4v" extension of the output
+ * files normally produced */
+ bool is_pipe = (strncmp(path, "pipe:", 5) == 0);
+ const char* format_name = is_pipe ? "ipod" : NULL;
+
Review Comment:
Is this an appropriate assumption to make universally, or should this be configurable, as well? Are there situations where users may want to stream in a format different than iPod?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "necouchman (via GitHub) via dev" <de...@guacamole.apache.org>.
necouchman commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-6547277976
@grintor Your third commit is not tagged appropriately. I'm pretty much ready to merge this, if you could either fix that commit message or squash 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub) via dev" <de...@guacamole.apache.org>.
grintor commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-6558678574
> @grintor Your third commit is not tagged appropriately. I'm pretty much ready to merge this, if you could either fix that commit message or squash it.
Ok, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub)" <gi...@apache.org>.
grintor commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-5682826448
Ok the commits are on top of the current main branch now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub)" <gi...@apache.org>.
grintor commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-5685268484
Good call on the exit code @vlouvet
I added a fix here for it.
Sorry to duplicate your work on GUACAMOLE-2298 I hadn't seen 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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#discussion_r3540563318
##########
src/guacenc/video.c:
##########
@@ -54,8 +56,16 @@ guacenc_video* guacenc_video_alloc(const char* path, const char* codec_name,
int ret;
int failed_header = 0;
+ /* The container format cannot be guessed from a pipe URL and must be
+ * specified explicitly. The "ipod" container is used, matching the
+ * container that would be guessed from the ".m4v" extension of the output
+ * files normally produced */
+ bool is_pipe = (strncmp(path, "pipe:", 5) == 0);
+ const char* format_name = is_pipe ? "ipod" : NULL;
+
Review Comment:
I agree that it was assumed by virtue of the fact that all files were necessarily `m4v` files. Now we're giving folks an option on what to name the files, which implicitly allows them to also adjust the format - and we're giving them an option to pipe everything to `stdout`, but forcing them to use the `ipod` format when going to `stdout`.
I'm not going to block this PR from going through if you don't want to do this, it's just something that seems to be in the same spirit as the ability to send it to `stdout`, and change the filename.
[It's also worth noting that simply having the ability to send to `stdout` means that you could also pipe it back into ffmpeg and reformat it that way, but you may start to lose the fidelity of the video that way due to lossy encodings, etc.]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub)" <gi...@apache.org>.
grintor commented on code in PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#discussion_r3539218378
##########
src/guacenc/video.c:
##########
@@ -54,8 +56,16 @@ guacenc_video* guacenc_video_alloc(const char* path, const char* codec_name,
int ret;
int failed_header = 0;
+ /* The container format cannot be guessed from a pipe URL and must be
+ * specified explicitly. The "ipod" container is used, matching the
+ * container that would be guessed from the ".m4v" extension of the output
+ * files normally produced */
+ bool is_pipe = (strncmp(path, "pipe:", 5) == 0);
+ const char* format_name = is_pipe ? "ipod" : NULL;
+
Review Comment:
"ipod" was already an assumption being made universally before, just not explicitly. When writing to a file, the third argument to avformat_alloc_output_context2() is NULL so libavformat guesses the container from the extension of the output path. Since that was hardcoded to "FILE.m4v" it was always making the same guess based on ".m4v" -- 'ipod'. This is a pretty universally supported format, even in web browsers, so I think it's a good default. Maybe someone will want it to be configurable in the future, but we can just wait for that feature request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub)" <gi...@apache.org>.
grintor commented on code in PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#discussion_r3539357690
##########
src/guacenc/guacenc.c:
##########
@@ -106,15 +120,27 @@ int main(int argc, char* argv[]) {
/* Get current filename */
const char* path = argv[i];
- /* Generate output filename */
- char out_path[4096];
- int len = snprintf(out_path, sizeof(out_path), "%s.m4v", path);
+ /* Use the explicitly-provided output path, if any, streaming to
+ * STDOUT if "-" was given as the output */
+ const char* out_path;
+ char out_buffer[4096];
+ if (out_file != NULL)
+ out_path = strcmp(out_file, "-") ? out_file : GUACENC_STDOUT_PATH;
+
+ /* Otherwise, generate output filename from input filename */
+ else {
+
+ int len = snprintf(out_buffer, sizeof(out_buffer), "%s.m4v", path);
+
+ /* Do not write if filename exceeds maximum length */
+ if (len >= sizeof(out_buffer)) {
+ guacenc_log(GUAC_LOG_ERROR, "Cannot write output file for "
+ "\"%s\": Name too long", path);
+ continue;
+ }
+
+ out_path = out_buffer;
Review Comment:
Yea, this isn't handling the user input, but the user input isn't going into a buffer anywhere, it's being passed directly in to guacenc_encode(). There, if the filename is too long, the actual file-open call fails and guacenc logs it as a failure "Error occurred while opening output file".
This check is solely for the rare edge case where the default "FILE.m4v" ends up more than 4096 characters deep in a directory tree. But I didn't add that logic, it was in the original code [here](https://github.com/apache/guacamole-server/pull/687/changes/6f40cb5b1b42bf5394773142514ca5f0001b076e#diff-05daedc99f9bd28adae51e5f02797ec783ae34b53b9e758cf83473e7a0c9f55dL113-L117)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "vlouvet (via GitHub)" <gi...@apache.org>.
vlouvet commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-5684359097
I built b248d24b on Ubuntu 24.04 (FFmpeg 6.1.1, libvpx 1.14.0) and ran it against a small synthetic recording and a 1280x720 one. Everything below was checked with ffprobe:
| Command | Result |
|---|---|
| `guacenc -s 320x240 in.guac` | mpeg4 in `in.guac.m4v`, same as before |
| `-c libvpx-vp9 -F webm` | VP9 in `in.guac.webm` |
| `-c libvpx -F webm` | VP8 in `in.guac.webm` |
| `-c libaom-av1 -F webm` | AV1 in `in.guac.webm` |
| `-c libx264 -F mp4` | H.264 in `in.guac.mp4` |
| `-c libvpx-vp9 -o out.webm` | WebM, guessed from the extension |
| `-c libvpx-vp9 -F webm -o -` | valid WebM on stdout |
| `-o -` (defaults) | fragmented MP4 on stdout |
| `-F notaformat` | rejected before encoding, exit 1 |
| `-c not_a_codec` | "Failed to locate codec", no file written |
| second run into an existing file | refused, original left untouched |
One thing I ran into: `guacenc -c libvpx-vp9 in.guac` with no `-F` still names the output `in.guac.m4v`, so the ipod muxer is picked and the header write fails:
```
[ipod @ ...] Could not find tag for codec vp9 in stream #0, codec not currently supported in container
guacenc: ERROR: Error occurred while writing output file header.
```
The man page does say the codec has to fit the container, so I don't think this needs to hold up the PR. Once it's merged I'd be glad to send a small follow-up under GUACAMOLE-2298 that picks the default extension from the codec when neither `-F` nor `-o` is given (VP8/VP9/AV1 to `.webm`), with unit tests.
Separately, and the same in 1.6.0: guacenc exits 0 even when every file fails (bad codec, codec/container mismatch, refusing to overwrite). That matters a bit more now that output can go to a pipe. I can open an issue for it if it's worth changing.
Thanks for adding the format option. It covers most of what I filed GUACAMOLE-2298 for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "necouchman (via GitHub) via dev" <de...@guacamole.apache.org>.
necouchman commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-6588827923
@grintor Sorry, just noticed that you have commits for three different Jira issues. Each pull request needs to be limited to a single Jira issue, so please limit this one to GUACAMOLE-1370 and open new pull requests for the other Jira issues you're working to resolve or implement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-1370: guacenc output to stdout. [guacamole-server]
Posted by "grintor (via GitHub) via dev" <de...@guacamole.apache.org>.
grintor commented on PR #687:
URL: https://github.com/apache/guacamole-server/pull/687#issuecomment-6607465916
Ok, I've limited to the pr to the first commit. Once it's merged, I'll open the other PRs since they build on this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org