Fix mediafoundation zero-copy capturer for cameras with no nv12 [chromium/src : main]
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron.
Ilya Nikolaevskiy would like Isuru Pathirana, Guido Urdaneta and Rafael Cintron to review this change.
Fix mediafoundation zero-copy capturer for cameras with no nv12
- Don't use GMB pool if format is not NV12
- Don't fillter out non-nv12 buffers if there is no nv12 support
Bug: 1214028
Change-Id: If81ca656be8217aa2f989f329c8693d1d7340785
---
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
3 files changed, 18 insertions(+), 8 deletions(-)
Rafael Cintron (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Christian Larson.
Rafael Cintron would like Christian Larson to review this change authored by Ilya Nikolaevskiy.
Fix mediafoundation zero-copy capturer for cameras with no nv12
- Don't use GMB pool if format is not NV12
- Don't fillter out non-nv12 buffers if there is no nv12 support
Bug: 1214028
Change-Id: If81ca656be8217aa2f989f329c8693d1d7340785
---
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
3 files changed, 18 insertions(+), 8 deletions(-)
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Christian Larson.
Patch set 1:Code-Review +1
6 comments:
Commit Message:
Patch Set #1, Line 10:
fillterNit: fillter -> filter
Patchset:
Adding Christian to the CL to get his feedback on the new logic.
File media/capture/video/win/video_capture_device_factory_win.cc:
// If we're using the hardware capture path, ignore non-NV12 pixel formats
// to prevent copies, if nv12 format is supported.If possible, please add a test which ensures the items in the list are what we expect based on this new logic.
seen_nv12 = true;
formats.clear();Apologies for not being familiar with the selection algorithm.
Is |formats| meant to be a sorted list worthy of consideration, with the first ones being the most favored? If so, would it make sense to sort the list after we've finished building it and put the NV12 ones first?
if (dxgi_device_manager_available && is_nv12 && !seen_nv12) {
seen_nv12 = true;
formats.clear();
}
// Skip this non-nv12 format.
if (dxgi_device_manager_available && !is_nv12 && seen_nv12)
continue;Based on the comment, seems like we can re-write this code to be:
if (dxgi_device_manager_available) {
if (is_nv12 && !seen_nv12) {
// We want to favor NV12 so remove all previous NV12 formats
// from consideration
seen_nv12 = true;
formats.clear();
} else if (!is_nv12 && seen_nv12) {
// If we've already seen NV12, do not consider any other type of
// format. Only NV12 formats are worth our time.
continue;
}
}
formats.push_back(capture_format);
File media/capture/video/win/video_capture_device_mf_win.cc:
selected_video_capability_->supported_format.pixel_format ==
PIXEL_FORMAT_NV12 &&Can you please add a comment here describing why need this particular if statement?
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Christian Larson.
Ilya Nikolaevskiy uploaded patch set #2 to this change.
Fix mediafoundation zero-copy capturer for cameras with no nv12
- Don't use GMB pool if format is not NV12
- Don't filter out non-nv12 buffers if there is no nv12 support
Bug: 1214028
Change-Id: If81ca656be8217aa2f989f329c8693d1d7340785
---
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
3 files changed, 18 insertions(+), 8 deletions(-)
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron, Christian Larson.
5 comments:
Commit Message:
Patch Set #1, Line 10:
fillterNit: fillter -> filter
Done
File media/capture/video/win/video_capture_device_factory_win.cc:
// If we're using the hardware capture path, ignore non-NV12 pixel formats
// to prevent copies, if nv12 format is supported.
If possible, please add a test which ensures the items in the list are what we expect based on this […]
Rafael, I need your help here please.
Actually, there's already a test for that [1]. Unfortunately, it doesn't do anything, because MFCreateSourceReaderFromMediaSource call [2] fails with "MFCreateSourceReaderFromMediaSource failed: No such interface supported (0x80004002)" error. Therefore the actual code below isn't executed and the resulting list of the supported formats is empty. The test just doesn't test if the result is empty and passes because of that. Should the test be fixed it would fail all the time.
I couldn't find MFCreateSourceReaderFromMediaSource source code. How does it produce IMFSourceReader from IMFMediaSource?
Are some method implementations missing from the StubMediaSource [3]?
With some debug logging I can see that MFCreateSourceReaderFromMediaSource does:
1) Call QueryInterface with some unimplemented in [3] uuid 10 times for 7 different UIIDs.
2) Calls GetSourceAttributes, which is not implemented
3) Calls GetStreamAttributes 2 times, which is not implemented
4) Calls CreatePresentationDescriptor, which is implemented.What interface does it try to cast the StubMediaSource to? Could the missing GetSourceAttributes cause it to fail?
I was able to print the list of UIIDs, but I couldn't convert that to actual interface types. Also, I don't know which of them actually break MFCreateSourceReaderFromMediaSource and which can be skipped. It would be a bummer if all 7 would have to be implemented. Here's the list:
{3C9B2EB9-86D5-4514-A394-F56664F9F0D8}
{3C9B2EB9-86D5-4514-A394-F56664F9F0D8}
{3C9B2EB9-86D5-4514-A394-F56664F9F0D8}
{7F02A37E-4E81-11E0-8F3E-D057DFD72085}
{FBB03414-D13B-4786-8319-5AC51FC0A136}
{2347D60B-3FB5-480C-8803-8DF3ADCD3EF0}
{03910848-AB16-4611-B100-17B88AE2F248}
{2CD0BD52-BCD5-4B89-B62C-EADC0C031E7D}
{2347D60B-3FB5-480C-8803-8DF3ADCD3EF0}
{03910848-AB16-4611-B100-17B88AE2F248}
seen_nv12 = true;
formats.clear();
Apologies for not being familiar with the selection algorithm. […]
I'm not sure how this list is used at the other side. It would be safer to remove everything other than NV12 if we want to ensure HW path being used.
if (dxgi_device_manager_available && is_nv12 && !seen_nv12) {
seen_nv12 = true;
formats.clear();
}
// Skip this non-nv12 format.
if (dxgi_device_manager_available && !is_nv12 && seen_nv12)
continue;
Based on the comment, seems like we can re-write this code to be: […]
Done
File media/capture/video/win/video_capture_device_mf_win.cc:
selected_video_capability_->supported_format.pixel_format ==
PIXEL_FORMAT_NV12 &&Can you please add a comment here describing why need this particular if statement?
Done
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Christian Larson (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Rafael Cintron.
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #3, Line 1036:
AddStreamIs the desired behavior for the MFCaptureEngine pipeline to do the color conversion if so you can convert to NV12 here. You just need to create a new media NV12 type with the same resolution and framerate.
You can see similar behavior in this sample except it is setting the type to RGB32
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron, Christian Larson.
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #3, Line 1036:
AddStream
Is the desired behavior for the MFCaptureEngine pipeline to do the color conversion if so you can co […]
Done. I had to change existing |GetMediaFormatConfigurationFromMFSourceMediaSubtype| routine. Now IMFCaptureEngine is configured to produce NV12 even if camera supports only I420.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron, Christian Larson.
1 comment:
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #3, Line 1036:
AddStream
Done. I had to change existing |GetMediaFormatConfigurationFromMFSourceMediaSubtype| routine. […]
Done
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Christian Larson.
Ilya Nikolaevskiy has uploaded this change for review.
Fix mediafoundation zero-copy capturer for cameras with no nv12
- Don't use GMB pool if format is not NV12
- Don't filter out non-nv12 buffers if there is no nv12 support
Bug: 1214028
Change-Id: If81ca656be8217aa2f989f329c8693d1d7340785
---
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_mf_win.cc
3 files changed, 34 insertions(+), 16 deletions(-)
Rafael Cintron (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Christian Larson.
Patch set 5:Code-Review +1
Christian Larson (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Guido Urdaneta, Rafael Cintron.
1 comment:
File media/capture/video/win/video_capture_device_factory_win.cc:
// If we're using the hardware capture path, ignore non-NV12 pixel formats
// to prevent copies, if nv12 format is supported.
Rafael, I need your help here please. […]
It looks like the implementation is missing the following QI for IMFMediaEventGenerator it is implemented so the QI should not be an issue.
https://docs.microsoft.com/en-us/windows/win32/medfound/media-source-object-model
I do not believe the other ones are required but here is some more info.Sample source https://github.com/microsoft/Windows-driver-samples/tree/master/general/SimpleMediaSource
3C9B2EB9-86D5-4514-A394-F56664F9F0D8
https://docs.microsoft.com/en-us/windows/win32/api/mfidl/nn-mfidl-imfmediasourceex2CD0BD52-BCD5-4B89-B62C-EADC0C031E7D
https://docs.microsoft.com/en-us/windows/win32/api/mfobjects/nn-mfobjects-imfmediaeventgenerator2347D60B-3FB5-480c-8803-8DF3ADCD3EF0
https://docs.microsoft.com/en-us/windows/win32/api/mfidl/nn-mfidl-imfrealtimeclient03910848-AB16-4611-B100-17B88AE2F248
https://docs.microsoft.com/en-us/windows/win32/api/mfidl/nn-mfidl-imfrealtimeclientex
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Isuru Pathirana, Guido Urdaneta, Rafael Cintron, Christian Larson.
Patch set 6:Commit-Queue +1
2 comments:
Patchset:
Chris, Guido, please review.
File media/capture/video/win/video_capture_device_factory_win.cc:
// If we're using the hardware capture path, ignore non-NV12 pixel formats
// to prevent copies, if nv12 format is supported.
It looks like the implementation is missing the following QI for IMFMediaEventGenerator it is implem […]
Thanks Christian, it works now: indeed QI for IMFMediaEventGenerator was missing.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Guido Urdaneta (Gerrit)
Attention is currently required from: Isuru Pathirana, Ilya Nikolaevskiy, Christian Larson.
Patch set 6:Code-Review +1
2 comments:
Patchset:
lgtm % nit, but wait for other the reviewers before landing
File media/capture/video/win/video_capture_device_factory_win.cc:
"remove all previous formats" instead of "all previous NV12 formats"?
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Ilya Nikolaevskiy, Christian Larson.
Ilya Nikolaevskiy removed Isuru Pathirana from this change.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Christian Larson.
1 comment:
File media/capture/video/win/video_capture_device_factory_win.cc:
"remove all previous formats" instead of "all previous NV12 formats"?
Done
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Christian Larson.
Patch set 9:Commit-Queue +1
1 comment:
Patchset:
Christian, ptal.
Previous fix introduced an issue: Because all the sink formats are set to NV12, some random source format was selected. Now capability selection logic also checks for source format to ensure that there's no unnecessary conversion.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Christian Larson (Gerrit)
Attention is currently required from: Ilya Nikolaevskiy.
2 comments:
File media/capture/video/win/video_capture_device_factory_win.cc:
const bool is_nv12 = capture_format.pixel_format == PIXEL_FORMAT_NV12;
if (dxgi_device_manager_available) {
if (is_nv12 && !seen_nv12) {
// We want to favor NV12 so remove all previous non-NV12 formats
// from consideration.
seen_nv12 = true;
formats.clear();
} else if (!is_nv12 && seen_nv12) {
// If we've already seen NV12, do not consider any other type of
// format. Only NV12 formats are worth our time.
Just because 1 NV12 type is supported does not mean that all nv12 types will match the resolution or framerate of the other types on the camera. For instance camera pipeline decodes MJPG types to NV12 in frameserver automatically to optimize sharing camera between multiple apps. Most USB cameras do not use MJPG for lower resolution types such as 480p and 320p since the usb bandwidth for USB2 is large enough to support YUY2 and that type would be skipped. Is this the intention?
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #9, Line 314:
TheseMight make it more readable to extend the format struct to contain hw vs software or have a separate list for SW and HW
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Christian Larson.
2 comments:
File media/capture/video/win/video_capture_device_factory_win.cc:
const bool is_nv12 = capture_format.pixel_format == PIXEL_FORMAT_NV12;
if (dxgi_device_manager_available) {
if (is_nv12 && !seen_nv12) {
// We want to favor NV12 so remove all previous non-NV12 formats
// from consideration.
seen_nv12 = true;
formats.clear();
} else if (!is_nv12 && seen_nv12) {
// If we've already seen NV12, do not consider any other type of
// format. Only NV12 formats are worth our time.
Just because 1 NV12 type is supported does not mean that all nv12 types will match the resolution or […]
Removed this code, since now regardless of the source format, the mf_sink_media_subtype is set to NV12 for hardware path. So in here actually all the formats would have NV12 if dxgi_device_manager_available==true.
To make sure we prioritize NV12 source format, now there's a change in capability_list_win.cc, where source format is taken into account when sorting the capabilities.
File media/capture/video/win/video_capture_device_mf_win.cc:
Patch Set #9, Line 314:
These
Might make it more readable to extend the format struct to contain hw vs software or have a separate […]
Done.
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Christian Larson (Gerrit)
Patchset:
Signoff from me. LGTM
File media/capture/video/win/video_capture_device_factory_win.cc:
const bool is_nv12 = capture_format.pixel_format == PIXEL_FORMAT_NV12;
if (dxgi_device_manager_available) {
if (is_nv12 && !seen_nv12) {
// We want to favor NV12 so remove all previous non-NV12 formats
// from consideration.
seen_nv12 = true;
formats.clear();
} else if (!is_nv12 && seen_nv12) {
// If we've already seen NV12, do not consider any other type of
// format. Only NV12 formats are worth our time.
Removed this code, since now regardless of the source format, the mf_sink_media_subtype is set to NV […]
Done
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Nikolaevskiy (Gerrit)
Attention is currently required from: Ilya Nikolaevskiy.
Patch set 11:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Fix mediafoundation zero-copy capturer for cameras with no nv12
- Don't use GMB pool if format is not NV12
- Don't filter out non-nv12 buffers if there is no nv12 support
Bug: 1214028
Change-Id: If81ca656be8217aa2f989f329c8693d1d7340785
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2944882
Commit-Queue: Ilya Nikolaevskiy <il...@chromium.org>
Reviewed-by: Guido Urdaneta <gui...@chromium.org>
Reviewed-by: Rafael Cintron <rafael....@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#893542}
---
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc
M media/capture/video/win/capability_list_win.cc
M media/capture/video/win/capability_list_win.h
M media/capture/video/win/video_capture_device_factory_win.cc
M media/capture/video/win/video_capture_device_factory_win_unittest.cc
M media/capture/video/win/video_capture_device_mf_win.cc
6 files changed, 184 insertions(+), 61 deletions(-)
To view, visit change 2944882. To unsubscribe, or for help writing mail filters, visit settings.