[media] Update MailboxConverter test to create SI with color space [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Looks like Dale is OOO, adding Colin for owners to take a look.
Related details
- Colin Blundell
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 2 comments![]( "Open in Gerrit")
Thanks!
// This test verifies that shared_image recreates if color space changes.Is this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
Related details
- Dale Curtis
- Saifuddin Hitawala
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. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
// This test verifies that shared_image recreates if color space changes.Is this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
This test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
Related details
- Colin Blundell
- Dale Curtis
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. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
// This test verifies that shared_image recreates if color space changes.Saifuddin HitawalaIs this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
This test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
This test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Hmm, so what's the utility of the test then if even before it wasn't actually testing what the test name says it was testing? i.e. is there a value to keeping this test at all at this point? or potentially scoping it down to be named as/targeted at the useful things it's actually testing?
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
- Dale Curtis
- Saifuddin Hitawala
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. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
// This test verifies that shared_image recreates if color space changes.Saifuddin HitawalaIs this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
Colin BlundellThis test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
This test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Hmm, so what's the utility of the test then if even before it wasn't actually testing what the test name says it was testing? i.e. is there a value to keeping this test at all at this point? or potentially scoping it down to be named as/targeted at the useful things it's actually testing?
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
That path sounds great to me!
Test is just very confusing, because the code flow is very confusing.
`VideoFrameConverter`'s purpose is to convert from `FrameResource` to `VideoFrame`. `FrameResource` is abstraction of the decoded buffer on the linux in chromeos.
MailboxVideoFrameConverter is what converts FrameResource to shared image backed VideoFrame, so it creates SharedImage from GpuMemoryBufferHandle provided by FrameResource.
Now, that even handle didn't change, we do need to recreate shared images if color space changes and this test tries to verify it.
To do so, test creates two `FrameResource` with different color spaces and expects MailboxVideoFrameConverter to recreate its SIs.
The rest is test specifics, there are few different ways to create `FrameResource`, normally this flow uses NativePixmap based FrameResource. This test for simplicity creates FrameResource from VideoFrame, because only ColorSpace matters. And to do so it creates SI.
So it looks wonky, but essentially:
```
SI => VideoFrame => FrameResource => Shared Image => VideoFrame
[ this is just test setup ][ this is what we test ]
```
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
// This test verifies that shared_image recreates if color space changes.Saifuddin HitawalaIs this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
Colin BlundellThis test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
Vasiliy TelezhnikovThis test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Hmm, so what's the utility of the test then if even before it wasn't actually testing what the test name says it was testing? i.e. is there a value to keeping this test at all at this point? or potentially scoping it down to be named as/targeted at the useful things it's actually testing?
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
That path sounds great to me!
Test is just very confusing, because the code flow is very confusing.
`VideoFrameConverter`'s purpose is to convert from `FrameResource` to `VideoFrame`. `FrameResource` is abstraction of the decoded buffer on the linux in chromeos.
MailboxVideoFrameConverter is what converts FrameResource to shared image backed VideoFrame, so it creates SharedImage from GpuMemoryBufferHandle provided by FrameResource.
Now, that even handle didn't change, we do need to recreate shared images if color space changes and this test tries to verify it.
To do so, test creates two `FrameResource` with different color spaces and expects MailboxVideoFrameConverter to recreate its SIs.
The rest is test specifics, there are few different ways to create `FrameResource`, normally this flow uses NativePixmap based FrameResource. This test for simplicity creates FrameResource from VideoFrame, because only ColorSpace matters. And to do so it creates SI.So it looks wonky, but essentially:
```
SI => VideoFrame => FrameResource => Shared Image => VideoFrame
[ this is just test setup ][ this is what we test ]
```
Got it, thanks - it's the converter's creation of a new SI that's being verified, and whether we're using one input SI or two input SIs to trigger the "color space changed" is just a test implementation detail.
Related details
- Dale Curtis
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 2 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
2 comments
Thanks for reviews!
// This test verifies that shared_image recreates if color space changes.Saifuddin HitawalaIs this test still testing what the comment says it's testing? Naively it seems like it isn't since we're explicitly creating the SharedImages now, but maybe I'm missing something. Is the idea that we're going to eliminate the behavior of changing the color space on the VideoFrame and make the color space immutable?
Colin BlundellThis test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
Vasiliy TelezhnikovThis test was not recreating the shared image earlier, but just setting the new color space on VideoFrame with shared image still having the old color space. While looking to add DumpWillBeCheck for mismatch I found this failing. I'm not exactly sure about the codepath flow here, maybe Vasiliy can clarify.
Hmm, so what's the utility of the test then if even before it wasn't actually testing what the test name says it was testing? i.e. is there a value to keeping this test at all at this point? or potentially scoping it down to be named as/targeted at the useful things it's actually testing?
Yes, we should make ColorSpace an immutable property of VideoFrame for when VideoFrame is created with SharedImage. This will then require recreating the SharedImage and hence such a VideoFrame on the client side. But right now there could be many such cases so we need to root out the ones with mismatches with DumpWillBeCheck. I found a couple places [here](https://chromium-review.googlesource.com/c/chromium/src/+/7657484) and [here](https://chromium-review.googlesource.com/c/chromium/src/+/7488606/4/media/video/video_encode_accelerator_adapter.cc) where I'm looking to recreate the SI.
That path sounds great to me!
Colin BlundellTest is just very confusing, because the code flow is very confusing.
`VideoFrameConverter`'s purpose is to convert from `FrameResource` to `VideoFrame`. `FrameResource` is abstraction of the decoded buffer on the linux in chromeos.
MailboxVideoFrameConverter is what converts FrameResource to shared image backed VideoFrame, so it creates SharedImage from GpuMemoryBufferHandle provided by FrameResource.
Now, that even handle didn't change, we do need to recreate shared images if color space changes and this test tries to verify it.
To do so, test creates two `FrameResource` with different color spaces and expects MailboxVideoFrameConverter to recreate its SIs.
The rest is test specifics, there are few different ways to create `FrameResource`, normally this flow uses NativePixmap based FrameResource. This test for simplicity creates FrameResource from VideoFrame, because only ColorSpace matters. And to do so it creates SI.So it looks wonky, but essentially:
```
SI => VideoFrame => FrameResource => Shared Image => VideoFrame
[ this is just test setup ][ this is what we test ]
```
Got it, thanks - it's the converter's creation of a new SI that's being verified, and whether we're using one input SI or two input SIs to trigger the "color space changed" is just a test implementation detail.
Thanks, that explanation makes so much sense!
Related details
- Dale Curtis
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
[media] Update MailboxConverter test to create SI with color space
Update MailboxVideoFrameConverterWithUnwrappedFramesTest to create
separate shared images and video frames for the test that creates
a second shared image with HDR color space.
- M media/gpu/chromeos/mailbox_video_frame_converter_unittest.cc
Code-Review: +1 by Colin Blundell, +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |