Include pixel type in layout messages triggered by `OnVideoSizeChanged`. [chromium/src : main]
Jamie Walch (Gerrit)
Jamie Walch added 1 comment![]( "Open in Gerrit")
This 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?
Related details
- Yuwei Huang
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. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
This 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?
Why 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.
Related details
- Jamie Walch
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. |
Lambros Lambrou (Gerrit)
Lambros Lambrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Code-Review | +1 |
1 comment
Include pixel type in layout messages triggered by `OnVideoSizeChanged`.You updated `OnDesktopDisplayChanged` as well. I think that's the important one - the `OnVideoSizeChanged` handler is only used in single-stream mode IIRC.
Related details
- Jamie Walch
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. |