Skip to content

[camera_web] Don't require enumerating cameras on web #6369

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

Closed

Conversation

kristoffer-zliide
Copy link

Enumerating available cameras on web requires permission and activates hardware. This PR allows initializing the camera controller without having to enumerate available cameras first.

flutter/flutter#145541

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

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

@kristoffer-zliide
Copy link
Author

At the time of this writing, a version of the example app where the enumeration of availableCameras have been replaced with

    _cameras = <CameraDescription>[
      const CameraDescription(
        name: 'Front',
        lensDirection: CameraLensDirection.front,
        sensorOrientation: 0,
      ),
      const CameraDescription(
        name: 'Back',
        lensDirection: CameraLensDirection.back,
        sensorOrientation: 0,
      ),
    ];

can be tried out here. Note that, contrary to the normal example app, it doesn't ask for permission before you tap on a camera.

Copy link

@QoLTech QoLTech left a comment

Choose a reason for hiding this comment

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

Yes, please.

We're creating an application that uses the camera and we're baffled that we need to show a permission dialog to get the list of camera devices. We also noticed that devices with many cameras take a longer time to initialize and display a preview.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in this file because the example had an existing issue, or is the web change breaking to clients?

@stuartmorgan-g
Copy link
Contributor

@kristoffer-zliide This appears to be failing web tests in CI (web_platform_tests_shard_2)

@ditman It looks like this is otherwise waiting for your review.

@stuartmorgan-g
Copy link
Contributor

From triage: @kristoffer-zliide Are you planning on fixing the failing tests in this PR?

@stuartmorgan-g
Copy link
Contributor

Marking as a draft pending resolution of the test failures.

@stuartmorgan-g stuartmorgan-g marked this pull request as draft June 17, 2024 19:46
@kristoffer-zliide kristoffer-zliide force-pushed the camera-web-initialization branch from 42480cf to 0e9c0fc Compare June 24, 2024 10:49
@kristoffer-zliide kristoffer-zliide force-pushed the camera-web-initialization branch 2 times, most recently from 6e06d43 to 6fbdca5 Compare July 26, 2024 11:06
@kristoffer-zliide kristoffer-zliide force-pushed the camera-web-initialization branch from 6fbdca5 to aafe0f2 Compare July 26, 2024 12:19
@kristoffer-zliide
Copy link
Author

@stuartmorgan: The tests are passing now.

@stuartmorgan-g stuartmorgan-g marked this pull request as ready for review July 30, 2024 18:32
@stuartmorgan-g
Copy link
Contributor

  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].

Please note that the PR won't be able to land without these steps, which is why they are in the checklist, and not doing it before review will add another round to the review process since things like the CHANGELOG wording are part of the review.

@ditman
Copy link
Member

ditman commented Aug 1, 2024

This will need to land until after the camera migration to package:web, but the transition to the new version shouldn't be too painful (🤞)

@stuartmorgan-g
Copy link
Contributor

@ditman Has that transition happened? If not, is there a tracking issue we can link to?

@ditman
Copy link
Member

ditman commented Sep 10, 2024

@stuartmorgan actually, yes, camera_web is in package:web since this PR:

@ditman
Copy link
Member

ditman commented Sep 10, 2024

After merging main, it turns out I don't have permissions to push changes to this PR:

ERROR: Permission to zliide/flutter_packages.git denied to ditman.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I'll try to refresh this branch so it can land. Thanks for the contribution @kristoffer-zliide, and apologies for the delay in the review! (x_x)


(@kristoffer-zliide I can move the changes to a new branch, if you'd prefer)

@stuartmorgan-g
Copy link
Contributor

@kristoffer-zliide Are you still planning on updating this PR based on the review feedback?

@kristoffer-zliide
Copy link
Author

I think I'm done here. Sorry if I wasted your time. Feel free to rebase/cherry-pick the four commits onto your own branches.

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.

4 participants