-
Notifications
You must be signed in to change notification settings - Fork 320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CameraCaptureUI API Implementation #4771
base: main
Are you sure you want to change the base?
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUIPhotoCaptureSettings.h
Show resolved
Hide resolved
|
||
namespace winrt::Microsoft::Windows::Media::Capture::factory_implementation | ||
{ | ||
struct CameraCaptureUIPhotoCaptureSettings : CameraCaptureUIPhotoCaptureSettingsT<CameraCaptureUIPhotoCaptureSettings, implementation::CameraCaptureUIPhotoCaptureSettings> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its been long enough that I forgot some details, but is this factory supposed to be non-agile so it has thread affinity? Or is it fine to be agile and the API is multi-thread safe it just needs to be called from a UI thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the factory can be designed to be agile, meaning it can operate across multiple threads, as long as it adheres to the requirement of being called from a UI thread.
Can you elaborate more on "thread affinity" in this context and your concerns regarding it.
{ | ||
void CameraCaptureUIVideoCaptureSettings::validate() | ||
{ | ||
float durationInMs = MaxDurationInSeconds * 1000.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these validate methods need STA thread type checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CameraCaptureUI API needs to be called from an STA thread. If it’s invoked from a non-STA thread, it will throw an exception: RPC_E_WRONG_THREAD. This occurs because accessing some UI elements or WinRT components which interact with UI is restricted to the thread that created it, which is typically the UI/STA thread.
I believe we don’t need an additional layer of checking or handling for this. The existing behavior sufficiently enforces the STA requirement.
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUI2.idl
Outdated
Show resolved
Hide resolved
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUI2.idl
Outdated
Show resolved
Hide resolved
<ClInclude Include="$(MSBuildThisFileDirectory)CameraCaptureUIVideoCaptureSettings.h" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<Midl Include="$(MSBuildThisFileDirectory)CameraCaptureUI2.idl" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 2 and not just CameraCaptureUI.idl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can stick with "CameraCaptureUI.idl" for simplicity. However, if we expect more modifications or want to distinguish it from the original, adding this version number might help with clarity.
(0x8ab25b76, 0xfe88, 0x4887, 0xb9, 0x08, 0x24, 0xf8, 0xca, 0x38, 0x2b, 0x9d)); | ||
//{ 8ab25b76-fe88-4887-b908-24f8ca382b9d } | ||
public: | ||
DEFINE_COMPLIANT_MEASURES_EVENT_PARAM2(CaptureInitiated, PDT_ProductAndServicePerformance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create an Activity instead to mark the start and end of the capture operation. Consider adding mode of capture in these events.
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUI.cpp
Outdated
Show resolved
Hide resolved
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUI.cpp
Outdated
Show resolved
Hide resolved
|
||
Uri launchUri{ L"microsoft.windows.camera.picker:" }; | ||
winrt::Windows::System::LaunchUriResult result = co_await Launcher::LaunchUriForResultsAsync(launchUri, options, properties); | ||
if (!result.Result()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check results.Status() before this?
bool isAppPackaged = m_telemetryHelper.IsPackagedApp(); | ||
PCWSTR appName = m_telemetryHelper.GetAppName().c_str(); | ||
try | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a lock to prevent/hold parallel calls to CaptureFileAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two (or more) consecutive CaptureFileAsync async calls run well, no change required.
Interested to know if this is a real use case for the developers since existing UWP API had issues (with lock) itself but not aware if there is bug raised for the same. Great to know if such feedback comes from the preview program. Can be taken up once the preview feedback is received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with current implementation without lock is that parallel calls to CaptureFileAsync API accidently overwrites the values of m_photoTokenFile
and m_videoTokenFile
member variables depending upon the mode of capture. That seems fundamentally wrong as the first call is not finished and tokens are already overwritten, and UWP API prevent that by lock (of-course that lock is not working as per expectation, needs to be fixed separately).
Either we can address this by modifying the API implementation and make it safe for parallel calls or write a lock and restrict it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If values are overridden, below piece of code will be executed and the result will be as expected.
file = co_await SharedStorageAccessManager::RedeemTokenForFileAsync(tokenValue);
Your concern is valid. However, as per the conversation with Saharsh, the API gives expected result so probably a follow up task or waiting to hear from preview feedback is ok.
If the below statement works well, why we need to have m_photoTokenFile
and m_videoTokenFile
member variables. Can we remove these two variable?
file = co_await SharedStorageAccessManager::RedeemTokenForFileAsync(tokenValue);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the Camera app has certain limitations, and the team recognized this when deciding to implement a lock to ensure only one capture operation occurs at a time on the original API. Even if multiple capture operations are initiated, only the focused camera will render, while others will show a message stating "Camera is already in use by another app."
Also, for real world use cases, if people do actually want multiple camera capture instances up, they should go through a feature request process.
For now, I am going ahead with the Locking mechanism and restricting this behavior just as it's done for Original API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea was discussed is can m_photoTokenFile
and m_videoTokenFile
be converted to local variable to avoid lock?
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUIVideoCaptureSettings.cpp
Outdated
Show resolved
Hide resolved
dev/Interop/CameraCaptureUI/CameraCaptureUI/CameraCaptureUIPhotoCaptureSettings.cpp
Outdated
Show resolved
Hide resolved
test/DynamicDependency/data/Microsoft.WindowsAppRuntime.Framework/appxmanifest.xml
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
namespace Microsoft.Windows.Media.Capture | ||
{ | ||
[contractversion(1)] | ||
apicontract CameraCaptureUIContract {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to add the contract "CameraCaptureUIContract" in specs/WinRT/WinRTAPIContracts.md as well.
concurrency::task<token_and_path> CreateEmptyFileAndGetToken(hstring const& fileExtension) | ||
{ | ||
auto tempFolder = ApplicationData::Current().TemporaryFolder(); | ||
auto tempFile = co_await tempFolder.CreateFileAsync(L"CCapture" + fileExtension, CreationCollisionOption::GenerateUniqueName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reasoning why the "CCapture" name is used here? I assume CCapture stands for CameraCapture. Can we use the CameraCapture sort of name instead? the reasoning behind that is it is better human readable name.
Having clear file name would help the program to determine the source of the file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. For better readability, I'll go ahead and use the full name instead.
} | ||
else | ||
{ | ||
file = co_await SharedStorageAccessManager::RedeemTokenForFileAsync(tokenValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can get the file name from the tokenValue itself, why you need to manage m_videoTokenFile and m_photoTokenFile? It would simplify your implementation.
catch (const std::exception& ex) | ||
{ | ||
CameraCaptureUITelemetry::CaptureError(isAppPackaged, appName, E_FAIL); | ||
throw hresult_error(E_FAIL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the pervious catch statement, throw ex is written. But in the current exception, hresult_error(E_FAIL) is written, curious to know the difference?
|
||
struct CameraCaptureUI : CameraCaptureUIT<CameraCaptureUI> | ||
{ | ||
CameraCaptureUI(winrt::Microsoft::UI::WindowId const& window) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Minor] Shouldn't window name be windowId? or Id? for improving code readability.
auto hasPhotoAspectRatioConstraint = (aspect.Width != 0) || (aspect.Height != 0); | ||
auto hasPhotoSizeConstraint = hasPhotoFixedSizeConstraint || hasPhotoAspectRatioConstraint; | ||
|
||
if (hasPhotoAspectRatioConstraint && hasPhotoAspectRatioConstraint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same condition twice
|
||
if (hasPhotoAspectRatioConstraint && hasPhotoAspectRatioConstraint) | ||
{ | ||
throw hresult_invalid_argument(L"PhotoSettings can't have both a size and aspect ratio specified"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should capture this information in the spec doc.
Don't specify both size and aspect ratio in the PhotoSettings, it will cause invalid argument exception
} | ||
else if (!AllowCropping && hasPhotoSizeConstraint) | ||
{ | ||
throw hresult_invalid_argument(L"PhotoSettings can't have a ratio or size specified with cropping disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as well we should capture in the spec doc.
Please add containment in the current PR or in a follow up PR. |
Validation Note: A microsoft employee must use /azp run to validate using the pipelines below.
API Spec: #4721
Fixes : #1034
API Design Overview
The CameraCaptureUI API allows desktop applications to effortlessly integrate native camera capture features across various Windows platforms. This proposal aims to address the limitations of the existing OS CameraCaptureUI, primarily designed for UWP, which poses challenges for desktop environments. Key issues include its reliance on CoreWindow and lack of support for IInitializeWithWindow, necessitating cumbersome workarounds.
This new API streamlines the integration of camera capture capabilities while ensuring feature parity across all Windows platforms supported by WinAppSDK, thus simplifying developers' workflows.
The CameraCaptureUI class provides a comprehensive UI for capturing audio, video, and photos. It allows customization of capture settings, including trimming video and adjusting camera settings.
Constructor:
Properties
PhotoSettings: Manages photo capture settings.
VideoSettings: Manages video capture settings.
Methods
This implementation ensures that the CameraCaptureUI API aligns closely with existing UWP functionality, only modifying the constructor to accept a WindowId.
Example C++ Code
This example shows how to use the CameraCaptureUI Class to take a picture.
Testing
Note: The Interactive Experiences package has been incorporated as a dependency in the Foundation repo to utilize Microsoft.UI API features.
Pending Action Items: