Include pixel type in layout messages triggered by `OnDesktopDisplayChanged`. [chromium/src : main]
Jamie Walch (Gerrit)
Jamie Walch added 2 comments![]( "Open in Gerrit")
Yuwei HuangThis fixes the bug, and it seems like it was an omission not to copy the pixel type into the protobuf so I think it's safe, but I don't really understand this code. Why do we have a separate internal representation of the video layout? It opens us to more bugs of this sort in the future.
Also, there are other call-sites that construct a `VideoLayout` proto and don't set the pixel type (for example, `ClientVideoDispatcher::OnIncomingMessage`). Do they also need to be updated?
Jamie WalchWhy do we have a separate internal representation of the video layout?
Maybe it was due to dependency or something. You have to ask Gary for the reason :)
`DesktopDisplayInfo` has a [GetVideoLayoutProto()](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/desktop_display_info.h;l=102;drc=cd4d28a2c2add65c76bb3252dc28370982577e33) method for the opposite conversion. Maybe it's a good idea to add a static `FromVideoLayoutProto` to make it more obvious that it needs to be changed whenever fields are added or removed.
Also, there are other call-sites that construct a VideoLayout proto and don't set the pixel type (for example, ClientVideoDispatcher::OnIncomingMessage). Do they also need to be updated?
Sounds like it should be updated, though looking at the age of that file, I suspect it is only used by chromotocol.
`DesktopDisplayInfo` has a [GetVideoLayoutProto()](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/desktop_display_info.h;l=102;drc=cd4d28a2c2add65c76bb3252dc28370982577e33) method for the opposite conversion. Maybe it's a good idea to add a static `FromVideoLayoutProto` to make it more obvious that it needs to be changed whenever fields are added or removed.
Done
> Also, there are other call-sites that construct a VideoLayout proto and don't set the pixel type (for example, ClientVideoDispatcher::OnIncomingMessage). Do they also need to be updated?Sounds like it should be updated, though looking at the age of that file, I suspect it is only used by chromotocol.
The other call-sites are `ClientVideoDispatcher::OnIncomingMessage` and `ClientSession::OnVideoSizeChanged`. Based on your and Lambros's comments, the former is Chromotocol-only and the latter is single-stream-only, so I don't think either needs to be updated.
Include pixel type in layout messages triggered by `OnVideoSizeChanged`.Jamie WalchYou updated `OnDesktopDisplayChanged` as well. I think that's the important one - the `OnVideoSizeChanged` handler is only used in single-stream mode IIRC.
I removed the OnVideoSizeChanged call-site and updated the CL description.
Related details
- Lambros Lambrou
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
2 comments
// For single-stream clients, the first layout must be the current webrtcNot for this CL, but are we going to remove the legacy message once single-stream is no longer supported? The discrepancy between the wire format and the actual layout is always causing headaches..
Related details
- Jamie Walch
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jamie Walch (Gerrit)
Jamie Walch added 1 comment![]( "Open in Gerrit")
// For single-stream clients, the first layout must be the current webrtcNot for this CL, but are we going to remove the legacy message once single-stream is no longer supported? The discrepancy between the wire format and the actual layout is always causing headaches..
The legacy message can probably be removed, but I'm pretty sure the website ignores it as soon as it's seen an extended message. The legacy _fields_ at the start of the extended message are harder to remove, because the client strips them so we'd need some way of knowing that the host had already done so, and there's already more special-case code based on host versions in the client than I'd like.
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jamie Walch (Gerrit)
Jamie Walch voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Include pixel type in layout messages triggered by `OnDesktopDisplayChanged`.
This is triggered after capabilities are negotiated and results in a layout message being sent to the client with no pixel type set, which breaks the cursor scaling (and probably some other things) if resize-to-fit is disabled (if it's enabled, then the desktop is resized almost immediately, resulting in an updated layout message). This CL refactors the proto->`DesktopDisplayInfo` conversion and updates it to include the pixel type. It also resets the pixel type in `DesktopDisplayInfo::Reset`, which isn't required for the bug-fix, but seems like the right thing to do.
BUG=481041921
- M remoting/host/client_session.cc
- M remoting/host/desktop_display_info.cc
- M remoting/host/desktop_display_info.h
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |