Vnc264

[berrange]

As defined by:

https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding

The noVNC HTML application recently added support for this encoding. There is
also an open pull request to add audio support to noVNC:

novnc/noVNC#1952

With that in place, the web based VNC console is good enough to display
a VM showing a video with reasonable bandwidth.

Possible improvements:

• Dynamic switching to/from H264 mode at high change rates
• do not compute all rects in vnc_update_client to reduce CPU load

We may also extend the RFB Audio protocol with "opus" encoding, because uncompressed
audio need too much bandwidth.

Summary by Sourcery

Add H.264 video encoding support for VNC using GStreamer, enabling more efficient video streaming over VNC connections

New Features:

• Implement H.264 video encoding for VNC using multiple GStreamer encoders
• Add dynamic encoder selection with support for nvh264enc, vaapih264enc, x264enc, and openh264enc
• Introduce configuration option to enable/disable H.264 encoding

Enhancements:

• Optimize VNC refresh mechanism to support H.264 encoding
• Add support for keeping dirty frames for smoother video transmission
• Implement flexible encoder initialization and context management

Build:

• Add GStreamer as an optional build dependency
• Update meson build configuration to support H.264 encoding

CI:

• Add GStreamer support to build options and configuration scripts

Summary by CodeRabbit

• New Features

  • Added optional H.264 video encoding support for VNC using GStreamer, improving video streaming performance and efficiency.
  • Introduced a new build option to enable or disable GStreamer-based H.264 encoding for VNC.
  • Added a VNC server option to configure H.264 encoder selection or disable H.264 support.
  • Enhanced VNC client negotiation and update logic to support H.264 encoding, including optimized update scheduling.

• Chores

  • Updated build scripts and help documentation to reflect new GStreamer and H.264 VNC options.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request implements the VNC H.264 encoding, enabling the server to stream video using the H.264 codec. It introduces new functions for H.264 initialization, encoding, and cleanup, and integrates with GStreamer for encoding. The changes also include a new VNC option to configure the H.264 encoder and modifications to the refresh logic to accommodate H.264 encoding requirements.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Implements the VNC H.264 encoding, allowing the server to send H.264 encoded video frames to the client.	Added VNC_ENCODING_H264 to the list of supported encodings. Added a new vnc_h264_send_framebuffer_update function to handle H.264 encoding. Added a new vnc_h264_clear function to release H.264 resources. Added a new vnc_h264_encoder_init function to initialize the H.264 encoder. Modified set_encodings to enable H.264 encoding if supported by the encoder list. Modified vnc_disconnect_finish to clear H.264 resources on disconnect. Modified vnc_refresh to conditionally keep the display dirty for H.264 encoding. Modified vnc_worker_thread_loop to use H.264 encoding. Added a new 'h264' option to the VNC options to configure the H.264 encoder. Added gstreamer dependency to meson build file. Added gstreamer support to vl.c. Added gstreamer option to meson options file. Added gstreamer support to ui/meson.build. Created a new file ui/vnc-enc-h264.c to implement H.264 encoding.	ui/vnc.c ui/vnc-jobs.c ui/vnc.h meson.build system/vl.c scripts/meson-buildoptions.sh meson_options.txt ui/meson.build ui/vnc-enc-h264.c

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @berrange - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider adding a configure option to control whether H.264 encoding is enabled at build time.
• It might be worth exploring the use of a circular buffer to avoid allocating memory for each frame.

Here's what I looked at during the review

• 🟡 General issues: 1 issue found
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟡 Complexity: 2 issues found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (complexity): Consider extracting the H264-specific logic into a helper function to simplify the vnc_refresh loop and reduce nesting depth, which improves readability and maintainability without altering functionality..

Extract the H264‐specific logic into a helper function. For example, you could create a function that handles the “keep dirty” update and returns the effective client dirty flag. One way to do this is:

#ifdef CONFIG_GSTREAMER
static int h264_handle_dirty(VncState *vs, int client_dirty, bool *global_keep_dirty) {
    if (!vs->h264) {
        return client_dirty;
    }
    if (client_dirty) {
        vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
    } else if (vs->h264->keep_dirty > 0) {
        client_dirty = 1;
        vs->h264->keep_dirty--;
    }
    if (vs->h264->keep_dirty > 0) {
        *global_keep_dirty = true;
    }
    return client_dirty;
}
#endif

Then, update your loop in vnc_refresh as follows:

QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
    int client_dirty = has_dirty;
#ifdef CONFIG_GSTREAMER
    client_dirty = h264_handle_dirty(vs, client_dirty, &keep_dirty);
#endif
    int count = vnc_update_client(vs, client_dirty);
    rects += count;

#ifdef CONFIG_GSTREAMER
    if (vs->h264 && !count && vs->h264->keep_dirty) {
        VncJob *job = vnc_job_new(vs);
        int height = pixman_image_get_height(vd->server);
        int width = pixman_image_get_width(vd->server);
        vs->job_update = vs->update;
        vs->update = VNC_STATE_UPDATE_NONE;
        vnc_job_add_rect(job, 0, 0, width, height);
        vnc_job_push(job);
    }
#endif
}

This refactoring keeps all functionality intact while reducing nesting in the critical loop.

[sourcery-ai]

issue (complexity): Consider extracting the common rectangle update logic into a helper function to avoid code duplication between the H264 and non-H264 encoding paths, and to improve readability and maintainability of the code..

Consider extracting the common update logic into helper functions so that both branches reuse the same code where possible. For example, you can factor the rectangle handling into a small routine and then call it in the non‑H264 branch. One possible refactoring is:

static int process_frame_rectangles(VncState *vs, VncJob *job) {
    int n_rectangles = 0;
    VncRectEntry *entry, *tmp;

    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
        int n;
        if (job->vs->ioc == NULL) {
            vnc_unlock_display(job->vs->vd);
            /* Copy persistent encoding data */
            vnc_async_encoding_end(job->vs, vs);
            return -1;  // signal disconnect
        }
        if (vnc_worker_clamp_rect(vs, job, &entry->rect)) {
            n = vnc_send_framebuffer_update(vs,
                                            entry->rect.x,
                                            entry->rect.y,
                                            entry->rect.w,
                                            entry->rect.h);
            if (n >= 0) {
                n_rectangles += n;
            }
        }
        g_free(entry);
    }
    return n_rectangles;
}

Then, in your worker thread loop, you can simplify the two branches:

vs.magic = VNC_MAGIC;
n_rectangles = 0;
vnc_write_u8(&vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
vnc_write_u8(&vs, 0);
saved_offset = vs.output.offset;
vnc_write_u16(&vs, 0);

vnc_lock_display(job->vs->vd);

if (vs.vnc_encoding == VNC_ENCODING_H264) {
    int width = pixman_image_get_width(vs.vd->server);
    int height = pixman_image_get_height(vs.vd->server);
    int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
    if (n >= 0)
        n_rectangles += n;
    /* Free all rectangles since we are using full frame update */
    VncRectEntry *entry, *tmp;
    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
        g_free(entry);
    }
} else {
    int n = process_frame_rectangles(&vs, job);
    if (n < 0)
        goto disconnected;
    n_rectangles += n;
}

trace_vnc_job_nrects(&vs, job, n_rectangles);
vnc_unlock_display(job->vs->vd);

/* Put n_rectangles at the beginning of the message */
vs.output.buffer[saved_offset]     = (n_rectangles >> 8) & 0xFF;
vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;

vnc_lock_output(job->vs);

This refactoring reduces duplicated rectangle processing logic while keeping all functionality intact.

[berrange]

This suggestion doesn't remove any duplication, as the new funtion is only called in 1 place.

[sourcery-ai]

I understand the context of the review comment and the subsequent response. Here's a thoughtful reply:

@berrange You make a fair point. The suggested refactoring might not significantly reduce code duplication if the extracted function is only called in one place.

The primary goal of my original suggestion was to improve code readability and maintainability, but if the proposed helper function doesn't provide clear benefits, it may indeed be unnecessary. In this case, keeping the current implementation might be more straightforward.

If you see opportunities for genuine code simplification or reduction of complexity in this section, I'm open to hearing your perspective. Sometimes, the most effective refactoring comes from the developers most familiar with the specific context and requirements of the code.

Would you be interested in discussing alternative approaches to improving the code structure or readability?

[berrange]

Hey @berrange - I've reviewed your changes - here's some feedback:

Overall Comments:

* Consider adding a configure option to control whether H.264 encoding is enabled at build time.

The patches already add a configure option

* It might be worth exploring the use of a circular buffer to avoid allocating memory for each frame.

Nope, this doesn't make sense.

Any other comments about correctness of the code ?

[berrange]

@CodiumAI-Agent /review

[CodiumAI-Agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Return type mismatch vnc_h264_encoder_init is declared to return bool but returns -1 on failure, which converts to true, so error paths are not correctly reported. encoder_name = get_available_encoder(vs->vd->h264_encoder_list); if (encoder_name == NULL) { VNC_DEBUG("No H264 encoder available.\n"); return -1; } Memory leak The GstBuffer created by gst_buffer_new_wrapped_full is never unreffed or freed after pushing, causing a buffer leak each frame. src_buffer = gst_buffer_new_wrapped_full( 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL); g_signal_emit_by_name( vs->h264->source, "push-buffer", src_buffer, &flow_ret); Uninitialized field The VncH264.keep_dirty field is never initialized in the encoder init, leading to undefined behavior in the refresh logic. /* Number of frames we send after the display is clean. */ #define VNC_H264_KEEP_DIRTY 10 typedef struct VncH264 { char *encoder_name; GstElement *pipeline, *source, *gst_encoder, *sink, *convert; size_t width; size_t height; guint keep_dirty; Notifier shutdown_notifier; } VncH264;

[berrange]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

This change set introduces optional H.264 video encoding for the VNC server using the GStreamer multimedia framework. The build system is updated to detect and configure GStreamer support, and a new build option is provided for enabling or disabling this feature. Integration with GStreamer is conditionally compiled and initialized at runtime. The VNC implementation is extended with new logic to negotiate, initialize, and utilize H.264 encoding for framebuffer updates, including support for encoder selection and handling of dirty regions. New source files and API declarations are added to support this functionality.

Changes

File(s)	Change Summary
meson.build, meson_options.txt, ui/meson.build	Added optional GStreamer dependency and build option; conditionally includes H.264 encoder source file for VNC when enabled.
scripts/meson-buildoptions.sh	Added command-line flags and help for enabling/disabling GStreamer support in the build.
system/vl.c	Conditionally includes and initializes GStreamer at program startup if enabled.
ui/vnc-enc-h264.c (new)	Implements H.264 VNC encoder using GStreamer, including encoder selection, pipeline management, and framebuffer encoding logic.
ui/vnc-jobs.c	Modifies VNC worker thread logic to handle H.264 encoding as a special case, sending full-frame updates and adjusting rectangle processing.
ui/vnc.c	Integrates H.264 encoding into VNC client negotiation, framebuffer update sending, refresh scheduling, and display option parsing.
ui/vnc.h	Adds data structures, constants, feature flags, and function declarations for H.264 encoding support in VNC.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant QEMU
    participant GStreamer
    participant VNC_Client

    User->>QEMU: Start QEMU with VNC + H264 option
    QEMU->>GStreamer: gst_init()
    QEMU->>VNC_Client: Negotiate encodings (includes H264)
    VNC_Client->>QEMU: Requests H264 encoding
    QEMU->>GStreamer: Setup encoder pipeline
    loop On framebuffer update
        QEMU->>GStreamer: Push raw frame data
        GStreamer-->>QEMU: Encoded H264 frame
        QEMU->>VNC_Client: Send H264 framebuffer update
    end
    User->>QEMU: Shutdown
    QEMU->>GStreamer: Cleanup pipeline

Loading

Poem

In the warren, code hops anew,
With GStreamer streams and H.264 in view.
VNC now sings a sharper tune,
Frames encoded, arriving soon!
Bunnies cheer for options bright,
And pipelines built just right—
Streaming dreams through the virtual night.
🥕✨

✨ Finishing Touches

• 📝 Generate Docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 7

🔭 Outside diff range comments (1)

ui/vnc-jobs.c (1)

196-209: 🛠️ Refactor suggestion

Thread-safety risk: raw pointer to VncH264 is shared without ownership rules

vnc_async_encoding_start() / _end() merely copy the raw VncH264 * pointer between the I/O thread and the worker thread. While this works today, both threads may concurrently dereference the same GStreamer objects (e.g. if the main thread calls vnc_h264_clear() while the worker is still running) which is undefined behaviour and will crash inside GStreamer.

A low-cost fix is to guard vs->h264 with the existing output_mutex or hand the ownership exclusively to the worker for the duration of the job.

 /* vnc_async_encoding_start */
-    local->h264 = orig->h264;
+    WITH_QEMU_LOCK_GUARD(&orig->output_mutex) {
+        local->h264 = orig->h264;
+        if (local->h264) {
+            g_atomic_int_inc(&local->h264->refcnt);
+        }
+    }

…and symmetrically dec-ref in _end().

♻️ Duplicate comments (2)

ui/vnc-enc-h264.c (1)

274-287: ⚠️ Potential issue

Return value breaks contract with header

vnc_h264_encoder_init() is declared as bool but returns -1 on error.
Every caller will interpret this as “success”. Return false instead.

-        VNC_DEBUG("No H264 encoder available.\n");
-        return -1;
+        VNC_DEBUG("No H264 encoder available.\n");
+        return false;
...
-    return true;
+    return true;   /* unchanged */

ui/vnc.c (1)

3243-3256: Refactor H.264 “keep-dirty” handling into a helper (comment repeated)

This block inlines state management for keep_dirty, increasing the nesting
and cognitive load inside vnc_refresh(). A prior review already suggested
extracting it into a helper (see earlier discussion); the advice is still
valid and would make both paths (H.264 vs. non-H.264) easier to follow.

🧹 Nitpick comments (4)

meson.build (1)

1352-1357: Verify and refine the gstreamer dependency detection logic.

The condition if not get_option('gstreamer').auto() or have_block will probe for GStreamer whenever block‐device support is enabled, but H.264 encoding is actually tied to VNC, not block I/O. Consider tightening this to only detect GStreamer when the user explicitly requests it (get_option('gstreamer').enabled()) or when VNC is enabled (get_option('vnc').enabled()).

scripts/meson-buildoptions.sh (1)

232-232: Reposition the help entry to maintain alphabetical order.

The new gstreamer entry appears at the end of the list, but help options are grouped alphabetically. To keep the UI consistent, move:

  -- gstreamer       gstreamer support (H264 for VNC)

into the g section (e.g., between glusterfs and gnutls).

ui/vnc.h (1)

49-52: Avoid leaking GStreamer types to public headers

<gst/gst.h> is included unconditionally whenever CONFIG_GSTREAMER is set, propagating heavy GStreamer macros to every translation unit that includes vnc.h. Wrapping all GStreamer-specific structs in an opaque forward declaration keeps compile times down and removes dependency surprises for downstream code.

Also applies to: 191-194, 242-253

ui/vnc-enc-h264.c (1)

357-385: Defensive coding: replace tight assert()s with runtime checks

The function aborts the whole QEMU process if the server size ever differs
from client_width/client_height. A mis-behaving client should not be
able to crash the hypervisor; return -1 and fall back to RAW instead.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1da8f3a and 17f0414.

📒 Files selected for processing (9)

• meson.build (3 hunks)
• meson_options.txt (1 hunks)
• scripts/meson-buildoptions.sh (2 hunks)
• system/vl.c (2 hunks)
• ui/meson.build (1 hunks)
• ui/vnc-enc-h264.c (1 hunks)
• ui/vnc-jobs.c (3 hunks)
• ui/vnc.c (7 hunks)
• ui/vnc.h (7 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

ui/vnc.c (3)

ui/vnc.h (6)

• vnc_h264_send_framebuffer_update (655-655)
• vs (72-72)
• vs (74-74)
• vs (76-80)
• vnc_h264_clear (656-656)
• vnc_h264_encoder_init (654-654)

ui/vnc-enc-h264.c (3)

• vnc_h264_send_framebuffer_update (302-387)
• vnc_h264_clear (389-401)
• vnc_h264_encoder_init (274-296)

ui/vnc-jobs.c (3)

• vnc_job_new (82-92)
• vnc_job_add_rect (94-109)
• vnc_job_push (111-121)

🪛 Cppcheck (2.10-2)

ui/vnc-enc-h264.c

[error] 48-48: syntax error

(syntaxError)

🔇 Additional comments (7)

meson_options.txt (1)

257-258: LGTM! The GStreamer option is correctly defined.

The new GStreamer option is properly defined as a feature type with an auto default value, which is consistent with other VNC-related options. The description clearly indicates its purpose for VNC H.264 encoding.

system/vl.c (2)

143-145: LGTM! Proper conditional inclusion of GStreamer headers.

The GStreamer header inclusion is correctly guarded with CONFIG_GSTREAMER preprocessor checks, ensuring it's only included when the feature is enabled.

---

2855-2857: LGTM! GStreamer initialization is correctly implemented.

The GStreamer initialization with gst_init(&argc, &argv) is properly guarded with CONFIG_GSTREAMER and placed early in the initialization sequence, which is important as it allows GStreamer to process any of its own command-line arguments.

ui/meson.build (1)

49-49: LGTM! Conditional addition of the H.264 encoder source.

The new source file vnc-enc-h264.c is correctly added to the VNC source set, conditionally included only when GStreamer is enabled. This follows the same pattern used for other conditional VNC features like SASL authentication.

meson.build (2)

2574-2575: Record GStreamer found status correctly.

Setting CONFIG_GSTREAMER to gstreamer.found() aligns with other feature flags. This will make the #ifdef CONFIG_GSTREAMER guard available in C code.

---

4848-4849: Include GStreamer in the configuration summary.

Adding {'gstreamer support': gstreamer} to summary_info follows the existing pattern for optional dependencies (e.g., zstd, qpl). This will print the detected GStreamer package or false if not found.

ui/vnc-enc-h264.c (1)

119-147: destroy_encoder_context() ignores NULL safety for partial init

If failure happens mid-initialisation some members may still be NULL.
gst_clear_object() already tolerates NULL, but gst_element_set_state
does not – guard the call:

if (h264->pipeline) {
    ...
}

[coderabbitai]

🛠️ Refactor suggestion

Regenerate this script instead of manual edits.

This file is auto‐generated by meson-buildoptions.py (“do not edit!”). Manual additions risk being overwritten. Please update meson-buildoptions.py (and/or meson_options.txt) to include gstreamer, then re-run the generator to produce these --enable-gstreamer/--disable-gstreamer entries.

[coderabbitai]

⚠️ Potential issue

Possible disconnection race is no longer handled for H.264 path

In the legacy branch we bail out if job->vs->ioc == NULL to avoid writing to a dead socket, yet the new H.264 branch omits this check. If the client disappears between the first ioc check (line 262) and this point the encoder may still produce data that will never be sent and leak buffers.

At minimum mirror the early-exit pattern used below:

if (job->vs->ioc == NULL) {
    vnc_unlock_display(job->vs->vd);
    vnc_async_encoding_end(job->vs, &vs);
    goto disconnected;
}

[coderabbitai]

⚠️ Potential issue

Public API returns bool – callers expect false, not -1

The declaration advertises a boolean result, but the implementation (ui/vnc-enc-h264.c) returns -1 on failure. In C this is equivalent to true, so error paths are silently treated as success. Please change the implementation to return false instead (and keep the success path returning true).

[coderabbitai]

⚠️ Potential issue

src_buffer is never unreffed – memory leak per frame

gst_buffer_new_wrapped_full() adds a reference which must be released
after the push succeeds. Without gst_buffer_unref(src_buffer) the
reference count is never decremented, leaking one buffer every screen
refresh.

 g_signal_emit_by_name(
     vs->h264->source, "push-buffer", src_buffer, &flow_ret);
+gst_buffer_unref(src_buffer);   /* drop our reference */

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	src_buffer = gst_buffer_new_wrapped_full(
	0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
	g_signal_emit_by_name(
	vs->h264->source, "push-buffer", src_buffer, &flow_ret);
	if (flow_ret != GST_FLOW_OK) {
	VNC_DEBUG("gst appsrc push buffer failed\n");
	return -1;
	}
	src_buffer = gst_buffer_new_wrapped_full(
	0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
	g_signal_emit_by_name(
	vs->h264->source, "push-buffer", src_buffer, &flow_ret);
	gst_buffer_unref(src_buffer); /* drop our reference */
	if (flow_ret != GST_FLOW_OK) {
	VNC_DEBUG("gst appsrc push buffer failed\n");
	return -1;
	}

[coderabbitai]

⚠️ Potential issue

vnc_h264_encoder_init() uses bool but returns -1 → error paths look like success

vnc_h264_encoder_init() is declared to return int/bool but its
implementation returns -1 on failure and true on success.
Because C treats every non-zero value as true, callers (like the block
below) treat the failure path as success and enable H.264 anyway.

-    if (encoder_name == NULL) {
-        VNC_DEBUG("No H264 encoder available.\n");
-        return -1;
+    if (!encoder_name) {
+        VNC_DEBUG("No H264 encoder available.\n");
+        return false;   /* ← failure */
     }
...
-    return true;

Update both the implementation and all call-sites (set_encodings(), etc.) to
treat false/0 as failure.
This prevents clients from negotiating a non-working H.264 stream.

Committable suggestion skipped: line range outside the PR's diff.

[coderabbitai]

⚠️ Potential issue

Guard H.264 case with CONFIG_GSTREAMER to avoid link-time breakage

vnc_h264_send_framebuffer_update() is only compiled when CONFIG_GSTREAMER
is defined (see ui/vnc-enc-h264.c).
When QEMU is built without GStreamer, the symbol is absent, and these
un-guarded lines will trigger an undefined-reference linker error.

+#ifdef CONFIG_GSTREAMER
     case VNC_ENCODING_H264:
         n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
         break;
+#endif

If you still want VNC_ENCODING_H264 numerically accepted when GStreamer is
disabled, fall back to VNC_ENCODING_RAW (or reject it) inside the #else.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	case VNC_ENCODING_H264:
	n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
	break;
	#ifdef CONFIG_GSTREAMER
	case VNC_ENCODING_H264:
	n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
	break;
	#endif

[coderabbitai]

⚠️ Potential issue

strcmp() on a possible NULL ‑ unchecked access can segfault

qemu_opt_get(opts, "h264") returns NULL when the option is omitted, but the
code immediately dereferences the pointer with strcmp().
Booting QEMU without an explicit h264= parameter will therefore crash.

const char *h264_opt = qemu_opt_get(opts, "h264");

/* Default: "on" when option is absent */
-if (!strcmp(h264_opt, "off")) {
+if (!h264_opt || !strcmp(h264_opt, "on")) {
+    vd->h264_encoder_list = "";          /* enable with default list */
+} else if (!strcmp(h264_opt, "off")) {
     vd->h264_encoder_list = NULL;        /* disable h264 */
-} else if  (!strcmp(h264_opt, "on")) {
-    vd->h264_encoder_list = "";          /* enable with default list */
 } else {
     /* assume this is a comma-separated encoder list */
     vd->h264_encoder_list = h264_opt;
 }

Add a unit/CLI test to ensure bare -vnc ...,h264 (or no parameter) no longer
crashes.

Committable suggestion skipped: line range outside the PR's diff.