packages icon indicating copy to clipboard operation
packages copied to clipboard

[camera] Support image streams on Windows platform

Open jlundOverlay opened this issue 1 year ago • 8 comments

Adds support to the to the camera plugin for streaming frames on windows as implemented in https://github.com/flutter/packages/pull/7067

Fixes https://github.com/flutter/flutter/issues/97542

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jlundOverlay avatar Jul 25 '24 23:07 jlundOverlay

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Jul 25 '24 23:07 flutter-dashboard[bot]

Are we better off just removing the assertion all together, seems redundant to limit what non federated packages can do?

jlundOverlay avatar Jul 25 '24 23:07 jlundOverlay

Are we better off just removing the assertion all together, seems redundant to limit what non federated packages can do?

Excellent question. Personally, I think it's good to keep these checks. This tells app developers if they use an API that's unsupported on a specific platform, which is a common mistake. If a federated platform package wants to add a feature, they can ask us to remove a particular check to allowlist them.

loic-sharma avatar Jul 25 '24 23:07 loic-sharma

Looks like this needs to be rebased and merge conflicts fixed. https://github.com/flutter/packages/pull/7067 has landed, so I'm assuming once that's done, we're close to landing this?

cbracken avatar Aug 16 '24 17:08 cbracken

@jlundOverlay Are you still planning on updating this PR?

stuartmorgan-g avatar Aug 27 '24 19:08 stuartmorgan-g

Have fixed the merge conflicts, should be good to go.

jlundOverlay avatar Aug 27 '24 23:08 jlundOverlay

Are we better off just removing the assertion all together, seems redundant to limit what non federated packages can do?

Excellent question. Personally, I think it's good to keep these checks. This tells app developers if they use an API that's unsupported on a specific platform, which is a common mistake. If a federated platform package wants to add a feature, they can ask us to remove a particular check to allowlist them.

We should fix this via this pattern which is our preferred approach; the hard-coded platform checks are a legacy of pre-federation approaches. Rather than update the hard-coding, the much better fix here would be to add a new platform interface method to query for streaming support, update the supported platforms to return true for it, and then update the app-facing package to require those minimum versions of the implementation packages (to avoid regressions) and gate on that check instead of platform.

stuartmorgan-g avatar Aug 28 '24 09:08 stuartmorgan-g

@jlundOverlay Are you still planning on updating this to address the issue with missing responses?

If not, we should revert the underlying PR, as that code is not correct, and would cause serious issues if ever exposed.

stuartmorgan-g avatar Oct 01 '24 19:10 stuartmorgan-g

Please keep making progress.

yoongyo avatar Oct 27 '24 14:10 yoongyo

@yoongyo it seems as the original author of patch was doing that as part of their job, and maybe they don't work anymore. I think it would be wise to look for someone, who can continue working on this.

Of course, writing this is easy, but I already tried to put this into my job's todo-list, although, in the end, we will not use this feature :frowning_face: .

gamelaster avatar Oct 27 '24 15:10 gamelaster

Closing in light of https://github.com/flutter/packages/pull/7951 and the lack of response here. If you decided to revisit this feature in the future, please feel free to open a new PR!

stuartmorgan-g avatar Oct 29 '24 11:10 stuartmorgan-g