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

Create TextureStream.md #2743

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Create TextureStream.md #2743

wants to merge 19 commits into from

Conversation

sunggook
Copy link
Member

@sunggook sunggook commented Sep 1, 2022

No description provided.

@david-risney david-risney self-requested a review September 1, 2022 22:18
@david-risney david-risney added the API Proposal Review WebView2 API Proposal for review. label Sep 1, 2022
@sunggook sunggook marked this pull request as ready for review September 4, 2022 04:36
Copy link
Contributor

@david-risney david-risney left a comment

Choose a reason for hiding this comment

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

Initial comments reading the spec. I'll look more on Monday.

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
@sunggook sunggook requested review from david-risney and removed request for bradp0721 September 27, 2022 21:29
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
@sunggook
Copy link
Member Author

I've addressed comments, please reactivate anything it isn't clear to you. Thanks a lot.

@david-risney
Copy link
Contributor

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
/// should register the textures again.
/// The event is triggered when all requests for given stream id closed
/// by the Javascript, or the host's Stop API call.
HRESULT add_StopRequested(
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be good to have this explained somewhere in the documentation.

It almost sounds like the start/stop thing is just simple messaging from JS to native and doesn't even have to be in this API. They could use window.chrome.webview.postMessage to send messages from JS to native asking to start/stop. But the 10s timeout thing you documented makes me think there is more to it. Or is the 10s just a recommendation that doesn't appear anywhere in the code?

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
@sunggook
Copy link
Member Author

There is a comment without 'reply' box, so copied it here
"It will be good to have this explained somewhere in the documentation.

It almost sounds like the start/stop thing is just simple messaging from JS to native and doesn't even have to be in this API. They could use window.chrome.webview.postMessage to send messages from JS to native asking to start/stop. But the 10s timeout thing you documented makes me think there is more to it. Or is the 10s just a recommendation that doesn't appear anywhere in the code?"

getTextureStream follows existing media device API of getUserMedia that follows 10s rule. When getUserMedia is called, it communicates with the utility service where it sets up camera and pumping the video frame. Overall, textureStream uses architecture (internal code) of the getUserMedia as much as possible (it also returns same object of MediaStream), buffer, start, stop, naming id, permission, etc.

@sunggook
Copy link
Member Author

I think I mentioned to change API name from RemoveBuffer to RemoveTextureBuffer in some resolution (but I couldn't find it somehow), I think it is better to keep RemoveBuffer name, instead edit the comment with TextureBuffer.

@sunggook
Copy link
Member Author

I've addressed but there might be something not resolved (editor work strangely), please reactivate comments that needs more attention. Thanks.

specs/APIReview_TextureStream.md Show resolved Hide resolved
const transformer = new TransformStream({
async transform(videoFrame, controller) {
// Delay frame 100ms.
await new Promise(resolve => setTimeout(resolve, 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 1s (1000ms). Per discussion this is in place of actually modifying the video frame. And when actually modifying a video frame it will take some amount of time.

We can replace this line with

let transformedVideoFrame = await appSpecificCreateTransformedVideoFrame(originalVideoFrame);

And move the delay into that method

function appSpecificCreateTransformedVideoFrame(originalVideoFrame) {
// At this point the app would create a new video frame based on the original
// video frame. For this sample we just delay for 1000ms and return the
// original.
await new Promise(resolve => setTimeout(resolve, 1000));
return originalVideoFrame;
}


// We can create new video frame and edit them, and pass them back here
// if needed.
controller.enqueue(videoFrame);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this see above

return S_OK;
}).Get(), &post_stop_token);

HRESULT CallWebView2API(bool createBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this function isn't used in the sample code. If we need to call the webview2 methods we should call them directly.

specs/APIReview_TextureStream.md Show resolved Hide resolved
/// 'd3dDevice' is used for creating shared IDXGI resource and NT shared
/// of it. The host should use Adapter of the LUID from the GetRenderAdapterLUID
/// for creating the D3D Device.
HRESULT CreateTextureStream(
Copy link
Contributor

Choose a reason for hiding this comment

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

From discussion, by the scenario we need one texturestream to be able to work with multiple CoreWebView2s. Since the texturestream isn't tied to one CoreWebView2 we should move these methods to the CoreWebView2Environment. On the environment you should be able to CreateTextureStream. But as you pointed out there is no connection required to particular CoreWebView2 objects, so there's no Add or Register method necessary. Just create the TextureStream with a particular name and then its available by that name in all the CoreWebView2s for that environment.

We didn't talk as much about the RenderAdapterLUID property or the RenderAdapterLUIDChanged event, but it sounds like maybe those also should move to the CoreWebView2Environment.

specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
@sunggook
Copy link
Member Author

Hi Dave, I've updated as much as possible based on discussion on last meeting. Thanks a lot.

Copy link
Contributor

@david-risney david-risney left a comment

Choose a reason for hiding this comment

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

Per discussion I'm going back through and updating the wording and such of the documentation. Please review everything to make sure what I've said is accurate. But I've also called out a few specific spots where I had to make up something. I'll continue more tomorrow or Fri for the rest of the document.

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
/// Remove listener for WebTextureStreamStopped event.
HRESULT remove_WebTextureStreamStopped([in] EventRegistrationToken token);
}
/// texture that the host writes to so that the Renderer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// texture that the host writes to so that the Renderer
/// A shared texture object used by ICoreWebView2TextureStream to represent a video frame. You can obtain them via `ICoreWebView2TextureStream::CreateTexture`, write to the underlying shared memory, and then have it rendered to a video stream with `ICoreWebView2TextureStream::PresentTexture`. See those methods for details.

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
/// The caller expected to open it with ID3D11Device1::OpenSharedResource1
/// and writes the incoming texture to it.
[propget] HRESULT Handle([out, retval] HANDLE* value);
/// Returns IUnknown type that could be query interface to IDXGIResource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns IUnknown type that could be query interface to IDXGIResource.
/// The texture as an `IUnknown` that supports the `IDXGIResource` interface. You can use the `IDXGIResource` interface to write your texture data to the texture. Do not change the texture after calling `ICoreWebView2TextureStream::PresentTexture` or you the frame may not be rendered and the `ICoreWebView2TextureStream ErrorReceived` event will be raised.

specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
specs/APIReview_TextureStream.md Outdated Show resolved Hide resolved
@sunggook
Copy link
Member Author

Thanks Dave for detailed comment, I've updated most of area as you suggested, otherwise replied comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants