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?

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?

Just a suggestion to reduce indentation.

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?

Why don't you use your shift defines here?

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).

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.

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

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.

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.

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.

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?

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.

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.

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.

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

Reworked. See comment associated with new update.

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

Reworked. See comment associated with new update.

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

Same here. Are those max values invalid or unsupported?

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;

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).

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

Moved both malloc/free to the write thread.

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.

You have to address this.

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.

Moved allocation to initialization and removed the assert.

Removed.