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

Strengthen frame readiness while testing fromPixels with video element #6751

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

hujiajie
Copy link
Contributor

@hujiajie hujiajie commented Aug 13, 2022

This fixes the error in WebGPU-backed unit tests:

OperationError: Failed to execute 'importExternalTexture' on 'GPUDevice': Failed to import texture from video element that doesn't have back resource.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@hujiajie
Copy link
Contributor Author

replaceHTMLVideoElementSource() in tfjs-data is exempted at the moment because it cannot be built on top of the proposed utils here. It might deserve a separate PR to deal with that.

@gyagp
Copy link

gyagp commented Aug 13, 2022

Can we further simplify the usage, like "await createAndWaitForVideo(src)"? Then you may hide all the details behind this function. Also for simplicity, we may not need to support multiple sources.

@hujiajie
Copy link
Contributor Author

The unit test removes the video element when it's finished, so for symmetry document.body.appendChild(video) also lives at the top level. Meanwhile, it reads the autoplay policy may cause video.play() to fail if the element is not yet on screen. I feel it inevitably breaks createAndWaitForVideo() apart.

@hujiajie hujiajie marked this pull request as draft August 14, 2022 12:14
@hujiajie hujiajie closed this Aug 15, 2022
This fixes the error in WebGPU-backed unit tests:

  OperationError: Failed to execute 'importExternalTexture' on
  'GPUDevice': Failed to import texture from video element that doesn't
  have back resource.
@hujiajie hujiajie reopened this Aug 15, 2022
@hujiajie hujiajie marked this pull request as ready for review August 15, 2022 09:28
@qjia7 qjia7 requested review from gyagp, pyu10055 and lina128 August 16, 2022 00:56
Copy link

@gyagp gyagp left a comment

Choose a reason for hiding this comment

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

According to browserstack tests, I think it's safe to use a single function. But anyway, it's already a good improvement comparing with exsiting code, so LGTM.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @lina128)

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks!

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @lina128)

@gyagp gyagp merged commit 8b40572 into tensorflow:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants