Skip to content
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

Camera helper WIP #1980

Merged
merged 71 commits into from
May 23, 2018
Merged

Camera helper WIP #1980

merged 71 commits into from
May 23, 2018

Conversation

skommireddi
Copy link
Contributor

@skommireddi skommireddi commented Apr 16, 2018

Issue: #

PR Type

What kind of change does this PR introduce?
Feature
Documentation content changes
Sample app changes

What is the current behavior?

What is the new behavior?

Added a new CameraHelper to the Toolkit Helpers
Added a sample page and docs to show the usage of Camera Helper
Added a CameraPreview XAML control to preview video from camera sources
Added a sample page and docs for CameraPreview Control

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current [Fall Creators Update (16299)]
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

REDMOND\sokommir and others added 9 commits April 10, 2018 11:30
- Styling for sample page
- Get software bitmap and hook it to the Image element
- Added placeholder documentation CamerHelper.md
- Added camera icon for helper
…Initialization and any error messages.

- Updated xaml to show error messages.
…a helper initialization

- Updated xaml sample page to hide preview controls etc when no sources are found
- fixing clean up resources exception when navigating away from page and no resources are found.
@dnfclas
Copy link

dnfclas commented Apr 16, 2018

CLA assistant check
All CLA requirements met.

using Windows.Media.Capture.Frames;
using Windows.Media.MediaProperties;

namespace Microsoft.Toolkit.Uwp.Helpers.CameraHelper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace should just be Microsoft.Toolkit.Uwp.Helpers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, remove CameraHelper from namespace

## Example

```csharp
todo- provide example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO ;)

| [Device family](http://go.microsoft.com/fwlink/p/?LinkID=526370) | Universal, 10.0.14393.0 or higher |
| --- | --- |
| Namespace | Microsoft.Toolkit.Uwp |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Nuget package link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what nuget package link?

_frameReader = await _mediaCapture.CreateFrameReaderAsync(_frameSource);
_frameReader.AcquisitionMode = MediaFrameReaderAcquisitionMode.Realtime;
_frameReader.FrameArrived += Reader_FrameArrived;
Debug.WriteLine($"Frame Reader created on source: {_frameSource.Info.Id}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove the debug write lines.

/// Initializes Media Capture settings and starts video capture using Frame Reader.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public async Task<CameraHelperResult> InitializeAndStartCapture(MediaFrameSourceGroup group)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these async methods have Async at the end of their names?

/// Clean up and dispose resources
/// </summary>
/// <returns>>A <see cref="Task"/> representing the asynchronous operation.></returns>
public async Task Cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these async methods have Async at the end of their names?

/// Returns first available Frame Source Group.
/// </summary>
/// <returns>>A MediaFrameSourceGroup<see cref="Task"/> representing the asynchronous operation.</returns>
public static async Task<MediaFrameSourceGroup> GetFirstAvailableFrameSourceGroup()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these async methods have Async at the end of their names?

Odonno
Odonno previously requested changes Apr 18, 2018
/// <summary>
/// Gets or sets message indicating information when camera helper initialization is not successful
/// </summary>
public string Message { get => message; set => message = value; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a simple AutoProperty with default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Changed it to auto-property with default value.

Refactored sample page to use the xaml control
Updated styling
…w Control

Added doc for Camera Helper class, updated code bind file and sample page
Merge from original to update fork with latest
# Conflicts:
#	Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj
if(result.Status == false)
{
// log the error
var errorMessage = result.Message;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no logging actually happening here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant as an informational message, will reword it.

/// <summary>
/// Gets or sets a value indicating whether camera helper initialization was successful or not
/// </summary>
public bool Status { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to something more indicative i.e. isInitialized

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or an Enum if there is more than two, and potentialy removing the Message property below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used message property so we could pass on the exception info to the user. I can make an enum and pass on the initialization error messages.

}

// Subscribe to the video frame as they arrive
cameraHelper.VideoFrameArrived += CameraHelper_VideoFrameArrived;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to hook a callback to this event if initialization failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If initialization failed, you wont get any frames.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

precisely, this is why I am asking and implicitly suggesting to not hook a callback unless initialization succeeds

// No frame source available.
}

private void CameraHelper_VideoFrameArrived(object sender, VideoFrameEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this method do?

Copy link
Contributor Author

@skommireddi skommireddi Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a way for user to get video frames after they select a camera source. The user can subscribe to CameraHelper VideoFrameArrived event to get frames in real time. This just shows example code of how they can get frames from the event args.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add comments in the body of the method to that effect (i.e. this is where code to handle the video frame lives)

{
/// <summary>
/// Gets or sets Video Frame</summary>
public VideoFrame VideoFrame { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to only expose a getter and make the setter private or internal for usage only by CameraHelper

Copy link
Contributor Author

@skommireddi skommireddi Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's a good suggestion, will change setter to internal

/// <summary>
/// Initializes a new instance of the <see cref="CameraHelper"/> class.
/// </summary>
public CameraHelper()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this does nothing, you could get rid of it and use a factory pattern to create and initialize asynchronously your CameraHelper instance instead

</ComboBox>
<TextBlock x:Name="CameraErrorTextBlock" Style="{StaticResource MessageStyle}" Margin="0,0,0,10" Visibility="Collapsed"></TextBlock>
<Button x:Name="CaptureButton" Content="Capture Video Frame" Margin="0,10" Click="CaptureButton_Click"></Button>
<Image x:Name="CurrentFrameImage" MinWidth="300" Width="400" HorizontalAlignment="Left"></Image>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using an Image, you can use a MediaPlayerElement XAML control and hook it to your source directly (like in my email) so you don't have to orchestrate rendering of the frames as they come in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would save you most of the code in your page below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a different example of using the Video Frames and displaying in an image using the Camera Helper directly. The CameraPreview sample page demonstrates what you mentioned and it uses the xaml control encapsulating the MediaPlayerElement. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep ok

VideoFrameArrived="CameraPreviewControl_VideoFrameArrived"
SoftwareBitmapArrived="CameraPreviewControl_SoftwareBitmapArrived"></controls:CameraPreview>
<TextBlock x:Name="ImageHeader" Text="Image" ></TextBlock>
<Image x:Name="CurrentFrameImage" MinWidth="300" Width="400" HorizontalAlignment="Left"></Image>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the goal of CameraPreview to preview the stream? why is there an image control as well in this page then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to show WinML usage example. The goal is to show how the user can subscribe and get video frames.


_videoPreviewErrorTextBlock.Text = result.Message;
_videoPreviewErrorTextBlock.Visibility = result.Status ? Visibility.Collapsed : Visibility.Visible;
_videoPreviewText.Visibility = !result.Status ? Visibility.Collapsed : Visibility.Visible;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of checking if(!result.Status == true), don't negate result.Status and invert your logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/// <summary>
/// Event raised when a new software bitmap arrives.
/// </summary>
public event EventHandler<SoftwareBitmap> SoftwareBitmapArrived;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as said previously, I would get rid of this one and keep only the other one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, there should be one event

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would mean I would return VideoMediaFrame instead of VideoFrame because I can get the software bitmap only from VideoMediaFrame. Let me know and I will make the change.

Copy link
Contributor

@LPBourret LPBourret Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can get a SoftwareBitmap from a VideoFrame: if its SoftwareBitmap is null, you can convert back and forth from Direct3DSurface to SoftwareBitmap easily. I'd would just send back VideoFrame, not SoftwareBitmap or VideoMediaFrame

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the software bitmap and Direct3DSurface are null when I try to get them video frame.

@@ -292,6 +293,8 @@
<Content Include="SamplePages\BluetoothLEHelper\BluetoothLEHelper.png" />
<Content Include="SamplePages\BackdropBlurBrush\BackdropBlurBrush.png" />
<Content Include="SamplePages\Blur\BlurBehavior.png" />
<Content Include="SamplePages\CameraPreview\Camera.png" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace these two with these (CameraPreview and CameraHelper respectively) :

cameracontrol

camerahelper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed icons

@@ -0,0 +1,40 @@
<Page xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be split into xaml bind file so it can be edited by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split into separate XAML bind file

/// <summary>
/// Event raised when a new software bitmap arrives.
/// </summary>
public event EventHandler<SoftwareBitmap> SoftwareBitmapArrived;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, there should be one event

_videoPreviewText = (TextBlock)this.GetTemplateChild("VideoPreviewTextBlock");
_videoPreviewErrorTextBlock = (TextBlock)this.GetTemplateChild("VideoPreviewErrorTextBlock");

_frameSourceGroupCombo.SelectionChanged += FrameSourceGroupCombo_SelectionChanged;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you unsubscribe from this event in the beginning of this method (in case it was subscribed in the previous template)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user changes the template, this could be null, make sure to check for null

<StackPanel Grid.Row="1" Orientation="Vertical" Margin="0,10">
<TextBlock x:Name="VideoPreviewTextBlock" Text="Video Preview" Margin="0,0,0,10"></TextBlock>
<TextBlock x:Name="VideoPreviewErrorTextBlock" Style="{StaticResource MessageStyle}" Margin="0,0,0,10" Visibility="Collapsed"></TextBlock>
<MediaPlayerElement x:Name="MediaPlayerElementControl" MinWidth="300" Width="400" PosterSource="ms-appx:///Assets/Photos/PreviewUnavailable.png" HorizontalAlignment="Left">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will crash if the developer does not have a png at "Assets/Photos/PreviewUnavailable.png" in their app. Either remove this completly or expose this as a property on your control and do TemplateBinding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

<RowDefinition Height="Auto"></RowDefinition>
<RowDefinition Height="Auto"></RowDefinition>
</Grid.RowDefinitions>
<ComboBox x:Name="FrameSourceGroupCombo" Header="Frame Source Group" HorizontalAlignment="Left" Width="Auto">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a ComboBox, what if we just have a simple toggle button on top of the MediaPlayerElement to toggle between sources? We should then have a bool dependancy property to enable and disable the button.

If the user wants to provide a ComboBox, can we expose the list of sources in a read only list?

/// <summary>
/// Gets or sets a value indicating whether camera helper initialization was successful or not
/// </summary>
public bool Status { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or an Enum if there is more than two, and potentialy removing the Message property below

/// <summary>
/// Helper class to get Frame Source Groups that can be used by Media Capture
/// </summary>
public static class FrameSourceGroupsHelper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this whole helper be part of the camera helper and automaticly do this in the inatialization when the camera helper is initialized, simplifying the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified and moved the code to Camera Helper to simplify initialization.

}
```

## Example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to follow the docs template for the documentation and inlcude properties (and any other missing sections)

</ComboBox.ItemTemplate>
</ComboBox>
<StackPanel Grid.Row="1" Orientation="Vertical" Margin="0,10">
<TextBlock x:Name="VideoPreviewTextBlock" Text="Video Preview" Margin="0,0,0,10"></TextBlock>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid having any text in the control. Instead I'd remove the two textblocks and let the developer handle how they want to show errors. Maybe we can fire an event on error for that purpose?

1. Moved logic from FrameSourceGroupsHelper into Camera Helper and minimalized calling logic to camera helper.
2. Modified Camera preview Control to use toggle button instead of combobox for frame sources
3. Modified to use only one event for frames arrived.
# Conflicts:
#	Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj
#	Microsoft.Toolkit.Uwp.SampleApp/SamplePages/samples.json
@nmetulev nmetulev changed the base branch from rel/3.0.0-preview to master May 18, 2018 18:38
@michael-hawker
Copy link
Member

Provided some feedback offline as was having trouble with GitHub.

skommireddi and others added 3 commits May 22, 2018 15:34
…y type instead of by name, that way even if the user changes name of control, preview page will still work.

Added TemplatePart attributes to Camera Preview Control
@nmetulev nmetulev added this to the 3.0 milestone May 23, 2018
/// <summary>
/// Camera Control to preview video. Can subscribe to video frames, software bitmap when they arrive.
/// </summary>
[TemplatePart(Name = "MediaPlayerElementControl", Type =typeof(MediaPlayerElement))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should be pulled out as a constant so we can use it here and when we grab it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

{
var cameraHelper = _cameraPreviewControl?.CameraHelper;
_cameraPreviewControl.PreviewFailed += CameraPreviewControl_PreviewFailed;
await _cameraPreviewControl.StartAsync(cameraHelper);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to expose the camera helper for the camera preview control? It should map those events and hide the internal implementation. Why is it the developer who need to provide this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camera Helper is exposed so that the user can access properties and events exposed directly from the helper. The developer can either provide a camera helper if they have one created with some settings or use StartAsync() which creates a default camera helper instance.


private async void Application_Suspending(object sender, SuspendingEventArgs e)
{
if (Frame.CurrentSourcePageType == typeof(CameraPreviewPage))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The control should handle it's own cleanup on app suspension and resuming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can do this later. It's fine for a first implementation.

/// <summary>
/// Camera Control to preview video. Can subscribe to video frames, software bitmap when they arrive.
/// </summary>
[TemplatePart(Name = "MediaPlayerElementControl", Type =typeof(MediaPlayerElement))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@nmetulev nmetulev dismissed stale reviews from Vijay-Nirmal and Odonno May 23, 2018 21:36

old review

@nmetulev nmetulev merged commit b520f44 into CommunityToolkit:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants