Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create TextureStream.md #2743
base: main
Are you sure you want to change the base?
Create TextureStream.md #2743
Changes from 1 commit
514839e
d896d6b
36a9128
e277953
d28f0a3
99ab9c0
bc5d267
a28ec23
0392021
cf1d3c2
b2b6932
c86906b
37ca44d
a8889e2
d8ffcb0
f01f594
4327402
370337a
fb9f997
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Calling this a texture stream seems confusing from the web side of things. There is already something called a texture in WebGL in the web world and this is not related to that. Since this is actually returning a MediaStream can we call this getMediaStream instead and also update the native API to be called something similar like CoreWebView2MediaStream?
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.
A bunch of my comments are applicable to the API surface not the sample code and are left unresolved. Can you look through them and reply to them please?
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.
video_tracks and videoTrack are unused?
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.
Is it easy to show where the video_id value is defined and that the element is a video element?
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 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.
Many calls throughout here are missing error handling - aren't checking the HRESULT return value. In our sample code we have a CHECK_HRESULT (or something like that) macro we use.
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.
Usually in WebView2, object creation happens on the CoreWebView2Environment as it acts sort of like a factory. Then the object may have additional initialization (like setting up event handlers) and then add or use the object with a CoreWebView2.
Here it looks like the Create method is on the CoreWebView2 and creating it also adds it (by the name parameter) to available texture streams for that CoreWebView2. Is the TextureStream tied to that CoreWebView2 in particular? Or can we move the TextureStream creation to the CoreWebView2Environment and have a separate method for 'adding' it to a CoreWebView2? If so this would have the benefits of:
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 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.
Is this method that we don't show what its doing, going to interact with the TextureStream by writing frames to it or something? If so, we should show that method or parts of that method. The sample should show you how to interact with the TextureStream.
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.
Similarly we should show what's happening in here. As is, I don't really know what I'm supposed to do with this event.
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 goal is for this to be code that shows how to use this api end to end. We should have working error handling. How would we expect errors like this to be handled? Displayed to user in native UI or web UI or something else?
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.
Need ``` to end code sample block here
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 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.
Is that only frames within this CoreWebView2? Or does it apply across any CoreWebView2 associated with that browser process?
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've had issues in the past with APIs intending to work with iframes that are a different origin than the top level document's origin not working in that case. Please make sure to test that
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.
Please document the manner in which it will 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.
How about naming this
AddAllowedOrigin
instead? (And the corresponding Remove method)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.
What happens if it takes longer than 10s? Is this a requirement by our code or just a suggested goal for end developers?
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.
What does it mean to fulfill the stream request? Is the JS getTextureStream call waiting until the first Present call before it returns the MediaStream?
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.
Similarly to above, Create* methods usually go on the CoreWebView2Environment. We should consider separating Create from the Add/Register like mentioned above. If we keep them together at the very least it should be named CreateAndAddBuffer or something like that. Just naming something Create doesn't suggest that there will be registration 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.
You call it a buffer in the parameter name and in the Create method name, but the type name is Texture. How about making the type name and Create name match
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.
Great note that this can be called on any thread. Please make sure all methods and properties that work on other threads are noted as working on other threads. Its stated generally that WebView2 methods only work on the WebView2 UI thread so we need to explicitly call out anything that doesn't work like that.
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.
What does GetAvailableBuffer do? Its documentation above needs to explain.
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 is SetBuffer and Present different methods? Why doesn't Present take the parameters of SetBuffer and present them?
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.
What is the point of the Stop method? Isn't the native code in charge of calling Present and CreateBuffer? Why does it call Stop versus just no longer streaming data to the TextureStream? Does this do something to the corresponding JavaScript objects?
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 comments doesn't seem to apply to add_TextureError does 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.
Event names should be verb phrases like ErrorOccurred or ErrorDetected
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 these are general TextureStream errors then we don't need the 'Texture' name prefix.
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.
What does update mean in this context? Is it obvious to someone familiar with D3D what Update would mean here? Can you be more specific in this comment about what Update means?
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 can be a property instead right?
[propget] HRESULT Buffer([out, retval] ICoreWebView2StagingTexture** value);
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.
And add the ``` to end the idl block here
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 document is missing the MIDL3 API definition and its missing the WinRT/.NET C# sample code