bhyve: support noVNC SetPixelFormat request

[mp]

Summary

The bhyve VNC server would ignore the SetPixelFormat message from the
VNC client. This change supports a limited implementation to detect
and reorder the colors such as requested from the noVNC client.

PR: 280984

[corvink]

usr.sbin/bhyve/rfb.c

352

Should we emit warning here? So if the client requests a different bit depth, the user gets a warning about it and won't get confused by the wrong VNC output?

364–367

Shouldn't we compare those values to our current settings e.g. rc->red_shift etc.? What happens if client changes the pixel format twice?

376–386

Just a suggestion to reduce indentation.

478

It's very inefficient to convert the pixel format everytime. Not sure if it's a noticeable performance issue. Is it too complicated to change the server format?

479–481

Why don't you use your shift defines here?

596

[mp]

Addressing review feedback from corvink

[mp]

Thank you for the review feedback. I think I address most of it.

usr.sbin/bhyve/rfb.c

352

I don't think this is necessary. We were ignoring it completely with the prior implementation and always forcing our server format. While there is flexibility in the protocol, really the majority of clients just have the normal RGB output (except noVNC that has BGR).

364–367

I reworked the code to pick up new shift values. But once we start transposing the colors, we will continue regardless. I didn't want to complicate free-ing the pixel buffer since it would need to coordinate with the writing thread. Really not worth the hassle.

376–386

I incorporated this into the code path with other changes. Thanks.

478

Understood. I'm not sure it is easy to change the server format. And this is only done when there is a non-server format request made. Otherwise it just sends out the gc buffer. I may look for a future improvement to use AVX to reduce this overhead.

479–481

Thank you, good point. I originally used the same form as the original code and then switched to defining the SHIFT's. I forgot about this one so it has been updated.

[corvink]

usr.sbin/bhyve/rfb.c

352

Well, you were confused about the prior implementation because it didn't warn you that it has ignored the SetPixelFormat command. So, it might be a good idea to improve that in your updated version and warn user when we're ignoring the command.

355–357

Sry, if I misunderstand this. I'm unfamiliar with the VNC code. pixfmt_msg was read from the client right? So, should we use nstoh here?

364–367

How is this intended to work? Imagine a client requests BGR. Then we're changing all rc->shift values. After some time, the client requests RGB for some reason. We would exit here without changing the rc->shift values. So, we're continuing sending in BGR format, wouldn't we? So, this check should be moved down after changing rc values.

474

If a client switches from BGR back to RGB, we could avoid rewriting the pixels by checking if the shift values match the server format here.

482–484

Is this function called in the same thread as the set_pixfmt function? If not, we require some kind of locking to read/write these shift values. I'm unfamiliar with the VNC code and don't know if it already holds a lock or not.

1360

That's unrelated and can get it's own commit.

[mp]

Addressing further review feedback

From the VNC clients I have looked at, SetPixelFormat is usually set
at the beginning of the session and not reverted. However, handling
switching between VNC clients with different formats did need further
adjustments. Using a pthread mutex in the main data path (the write
thread) would have an adverse effect on performance. The approach I
took was to use an atomic_bool to notify the write thread of new values
and new value struct that is protected by a pthread mutex.

I also added warnings suggested in the review when invalid values are
sent via SetPixelFormat. Note: one of the VNC clients I tested with
will initially send a bpp value (8) which emits a warning now and
the client sends a subsequent valid bpp value (32). Hopefully users
are not confused by this warning.

[mp]

usr.sbin/bhyve/rfb.c

364–367

Reworked. See comment associated with new update.

474

If our server format is requested again, the new version will NULL out pixrow so the adjustment won't happen.

482–484

Reworked. See comment associated with new update.

[corvink]

usr.sbin/bhyve/rfb.c

352

Nit: The pixfmt is unsupported not invalid, isn't it?

363

Same here. Are those max values invalid or unsupported?

381–383

It may make sense to update these values with pixfmt_mtx held. When SetPixelFormat is called twice very fast (I know it's just a theoretical scenario), we would end up with the same race as before. A proper solution might be:

	if (red_shift == PIXEL_RED_SHIFT &&
	    green_shift == PIXEL_GREEN_SHIFT &&
	    blue_shift == PIXEL_BLUE_SHIFT) {
		pixelp = NULL;
	} else {
		/* write thread adjusts a row at a time */
		pixelp = malloc(RFB_MAX_WIDTH * sizeof(uint32_t));
		if (pixelp == NULL) {
			WPRINTF(("rfb: could not allocate pixel array"));
			return;
		}
	}

	pthread_mutex_lock(&rc->pixfmt_mtx);
	rc->new_pixfmt.red_shift = red_shift;
	rc->new_pixfmt.green_shift = green_shift;
	rc->new_pixfmt.blue_shift = blue_shift;
	if (rc->new_pixfmt.pixrow)
		free(rc->new_pixfmt.pixrow);
	rc->new_pixfmt.pixrow = pixelp;
	pthread_mutex_unlock(&rc->pixfmt_mtx);

	rc->update_pixfmt = true;

393

Theoretically, when updating the pixfmt very fast, we could end up allocating the pixrow buffer twice without freeing the first one.

It might be a better idea to let the write thread set pixrow to NULL when it picks it up. So, here we can free any non NULL value (see suggestion above).

692

Is this function called by multiple threads? Otherwise, this mtx is useless in your current approach.

1351–1353

[mp]

Address review feedback

• Changed warning from "invalid" to "unsupported"
• Double checked mutex handling
• Moved malloc/free to write thread
• Note: we could consider not free'ing the buffer once allocated as it is only ~8k as we already keep it around between sessions.

[mp]

usr.sbin/bhyve/rfb.c

393

Moved both malloc/free to the write thread.

[corvink]

usr.sbin/bhyve/rfb.c

661–671

I'm fine with allocating the buffer once. If you like to change it, just allocate the buffer on init to make it even simpler.

1351–1353

You have to address this.

[mp]

Address review feedback

• Remove pixmap malloc/free from write thread and malloc once at initialization similar to the zbuf malloc.
• In error handling, add pthread_mutex_destroy.

[mp]

@corvink thank you for the reviews. Hopefully these last changes should address all of your feedback.

usr.sbin/bhyve/rfb.c

1360

While I understand changes should be related to the issue being fixed, I think this one is fine to include since a similar line is being added next to it.

[corvink]

In D46402#1059143, @mp wrote:

@corvink thank you for the reviews. Hopefully these last changes should address all of your feedback.

Thanks for your contribution! I'm going to test this before merging.

usr.sbin/bhyve/rfb.c

1156

I see that you copied this from the zbuf. However, that's not correct. Commonly assertions are removed in release build. We don't want to continue on a failed allocation in release builds! I know that bhyve enables assertions even in release builds but that's a bug and we should ideally change that behavior in the future. Therefore, we should avoid making this migration harder by making our code even more depend on assertions.

1360

From my experience, FreeBSD isn't strict on that, so I'm fine with it.

Personally, I try to avoid such changes because they can be confusing and blow up your commit, making it harder to review and to analyze when hunting bugs. Just image what happens if zbuf isn't initialized to NULL causing free to be called on a random value under some conditions. After noticing that something is wrong and performing a git bisect, you end up with this commit. It may take some time until you notice that this commit not just adds supports for different PixelFmts. It's also fixing a different bug but you have to find that single line in your bigger overall change.

[mp]

Address review feedback

• Moved pixrow initialization and removed associated assert
• Removed extraneous free (will address later when removing another assert)

[mp]

@corvink your last comment included "I'm going to test this before merging.". I'm assuming you meant "before accepting" and then I will merge onto main and push the commit.

usr.sbin/bhyve/rfb.c

1156

Moved allocation to initialization and removed the assert.

1360

Removed.