blink: Fix squashed image when copying video frames [chromium/src : main]
Vikram Pasupathy (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Ted (Chromium) Meyer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 3 comments![]( "Open in Gerrit")
media_video_frame->metadata().transformation = transform;AFAIK, media_video_frame is shared across threads (the compositor uses it), and this might have some data race implications. It might also be fine, since the frame would have had to be rendered before it can be clicked on for this operation by the user, but I suppose there could be some weirdness lurking here. You can use VideoFrame::WrapVideoFrame to make a shallow copy and set the metadata there, I think.
wmp_natural_size = wmp->NaturalSize();
// The underlying VideoFrame may have been stripped of its transformation
// metadata by the decoder or hardware buffer pipeline. If the frame lacks
// a rotation but the WebMediaPlayer's pipeline metadata knows it exists,
// we must inherit it.
if (transform.rotation == media::VIDEO_ROTATION_0) {
transform = wmp->GetVideoTransformation();
media_video_frame->metadata().transformation = transform;
}
if (media_video_frame->visible_rect().size() == wmp_natural_size &&
(transform.rotation == media::VIDEO_ROTATION_90 ||
transform.rotation == media::VIDEO_ROTATION_270)) {
// Clear the transformation metadata to prevent double rotation during
// paint
media_video_frame->metadata().transformation = media::kNoTransformation;
transform = media::kNoTransformation;
}Should you just move this into condition on line 876?
if (!raster_context_provider) {
if (auto wrapper = SharedGpuContext::ContextProviderWrapper()) {
raster_context_provider =
wrapper->ContextProvider().RasterContextProvider();
}
}This looks identical to line 885-889, is RasterContextProvider() expected to return null the first time and non-null after that?
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 3 comments![]( "Open in Gerrit")
media_video_frame->metadata().transformation = transform;AFAIK, media_video_frame is shared across threads (the compositor uses it), and this might have some data race implications. It might also be fine, since the frame would have had to be rendered before it can be clicked on for this operation by the user, but I suppose there could be some weirdness lurking here. You can use VideoFrame::WrapVideoFrame to make a shallow copy and set the metadata there, I think.
Done
wmp_natural_size = wmp->NaturalSize();
// The underlying VideoFrame may have been stripped of its transformation
// metadata by the decoder or hardware buffer pipeline. If the frame lacks
// a rotation but the WebMediaPlayer's pipeline metadata knows it exists,
// we must inherit it.
if (transform.rotation == media::VIDEO_ROTATION_0) {
transform = wmp->GetVideoTransformation();
media_video_frame->metadata().transformation = transform;
}
if (media_video_frame->visible_rect().size() == wmp_natural_size &&
(transform.rotation == media::VIDEO_ROTATION_90 ||
transform.rotation == media::VIDEO_ROTATION_270)) {
// Clear the transformation metadata to prevent double rotation during
// paint
media_video_frame->metadata().transformation = media::kNoTransformation;
transform = media::kNoTransformation;
}Should you just move this into condition on line 876?
Good point, but `will_hard_flip` depends on checking the RasterContextProvider first. If I move this up, I'd have to drag all the GPU context setup up with it, which makes the code pretty messy. WDYT?
if (!raster_context_provider) {
if (auto wrapper = SharedGpuContext::ContextProviderWrapper()) {
raster_context_provider =
wrapper->ContextProvider().RasterContextProvider();
}
}This looks identical to line 885-889, is RasterContextProvider() expected to return null the first time and non-null after that?
- Ted (Chromium) Meyer
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. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 2 comments![]( "Open in Gerrit")
wmp_natural_size = wmp->NaturalSize();
// The underlying VideoFrame may have been stripped of its transformation
// metadata by the decoder or hardware buffer pipeline. If the frame lacks
// a rotation but the WebMediaPlayer's pipeline metadata knows it exists,
// we must inherit it.
if (transform.rotation == media::VIDEO_ROTATION_0) {
transform = wmp->GetVideoTransformation();
media_video_frame->metadata().transformation = transform;
}
if (media_video_frame->visible_rect().size() == wmp_natural_size &&
(transform.rotation == media::VIDEO_ROTATION_90 ||
transform.rotation == media::VIDEO_ROTATION_270)) {
// Clear the transformation metadata to prevent double rotation during
// paint
media_video_frame->metadata().transformation = media::kNoTransformation;
transform = media::kNoTransformation;
}Vikram PasupathyShould you just move this into condition on line 876?
Good point, but `will_hard_flip` depends on checking the RasterContextProvider first. If I move this up, I'd have to drag all the GPU context setup up with it, which makes the code pretty messy. WDYT?
I'm not sure I see the messy aspect? Wouldn't it just be moving line 886-892 up earlier? I guess it's an expensive setup that's pre-empted by the early return, but how often does that early return actually fire in real usage?
if (!raster_context_provider) {
if (auto wrapper = SharedGpuContext::ContextProviderWrapper()) {
raster_context_provider =
wrapper->ContextProvider().RasterContextProvider();
}
}Vikram PasupathyThis looks identical to line 885-889, is RasterContextProvider() expected to return null the first time and non-null after that?
Removed the redundant check! Done
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 2 comments![]( "Open in Gerrit")
wmp_natural_size = wmp->NaturalSize();
// The underlying VideoFrame may have been stripped of its transformation
// metadata by the decoder or hardware buffer pipeline. If the frame lacks
// a rotation but the WebMediaPlayer's pipeline metadata knows it exists,
// we must inherit it.
if (transform.rotation == media::VIDEO_ROTATION_0) {
transform = wmp->GetVideoTransformation();
media_video_frame->metadata().transformation = transform;
}
if (media_video_frame->visible_rect().size() == wmp_natural_size &&
(transform.rotation == media::VIDEO_ROTATION_90 ||
transform.rotation == media::VIDEO_ROTATION_270)) {
// Clear the transformation metadata to prevent double rotation during
// paint
media_video_frame->metadata().transformation = media::kNoTransformation;
transform = media::kNoTransformation;
}Vikram PasupathyShould you just move this into condition on line 876?
Ted (Chromium) MeyerGood point, but `will_hard_flip` depends on checking the RasterContextProvider first. If I move this up, I'd have to drag all the GPU context setup up with it, which makes the code pretty messy. WDYT?
I'm not sure I see the messy aspect? Wouldn't it just be moving line 886-892 up earlier? I guess it's an expensive setup that's pre-empted by the early return, but how often does that early return actually fire in real usage?
Like so?
if (!raster_context_provider) {
if (auto wrapper = SharedGpuContext::ContextProviderWrapper()) {
raster_context_provider =
wrapper->ContextProvider().RasterContextProvider();
}
}Vikram PasupathyThis looks identical to line 885-889, is RasterContextProvider() expected to return null the first time and non-null after that?
Ted (Chromium) MeyerRemoved the redundant check! Done
Missing a patch set? This looks unchanged
- Ted (Chromium) Meyer
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. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ted (Chromium) Meyer
| Code-Review | +1 |
2 comments
wmp_natural_size = wmp->NaturalSize();
// The underlying VideoFrame may have been stripped of its transformation
// metadata by the decoder or hardware buffer pipeline. If the frame lacks
// a rotation but the WebMediaPlayer's pipeline metadata knows it exists,
// we must inherit it.
if (transform.rotation == media::VIDEO_ROTATION_0) {
transform = wmp->GetVideoTransformation();
media_video_frame->metadata().transformation = transform;
}
if (media_video_frame->visible_rect().size() == wmp_natural_size &&
(transform.rotation == media::VIDEO_ROTATION_90 ||
transform.rotation == media::VIDEO_ROTATION_270)) {
// Clear the transformation metadata to prevent double rotation during
// paint
media_video_frame->metadata().transformation = media::kNoTransformation;
transform = media::kNoTransformation;
}Vikram PasupathyShould you just move this into condition on line 876?
Ted (Chromium) MeyerGood point, but `will_hard_flip` depends on checking the RasterContextProvider first. If I move this up, I'd have to drag all the GPU context setup up with it, which makes the code pretty messy. WDYT?
Vikram PasupathyI'm not sure I see the messy aspect? Wouldn't it just be moving line 886-892 up earlier? I guess it's an expensive setup that's pre-empted by the early return, but how often does that early return actually fire in real usage?
Like so?
it seems fine to me now.
if (!raster_context_provider) {
if (auto wrapper = SharedGpuContext::ContextProviderWrapper()) {
raster_context_provider =
wrapper->ContextProvider().RasterContextProvider();
}
}Vikram PasupathyThis looks identical to line 885-889, is RasterContextProvider() expected to return null the first time and non-null after that?
Ted (Chromium) MeyerRemoved the redundant check! Done
Vikram PasupathyMissing a patch set? This looks unchanged
oops. PTAL
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |