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

Can't upload video from camera roll #3624

Closed
maltokyo opened this issue Sep 20, 2019 · 18 comments · Fixed by #5734
Closed

Can't upload video from camera roll #3624

maltokyo opened this issue Sep 20, 2019 · 18 comments · Fixed by #5734
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending

Comments

@maltokyo
Copy link

maltokyo commented Sep 20, 2019

Only can upload photos. Can't upload any videos or other filetypes.
(even in the beta version, which I tried also)

[edit: this report was on iOS; edited title based on follow-up report in later comment.]

[edit: see below for a proposed solution that should hopefully be pretty easy?]

@astyltsvig
Copy link

I cant either do that from android

@gnprice
Copy link
Member

gnprice commented Jan 21, 2020

Hi! Thanks for the reports.

I just tested, and this works for me. (On Android 10 with v26.20.143, the latest.) @maltokyo @astyltsvig can you say in more detail what steps you try, and what result you see?

One thing that may confound this is #2684. Videos tend to be much bigger than photos, so they can naturally take some time to upload (often like 10 seconds, potentially much longer), depending on the speed of your connection to the Internet and to your Zulip server. We ought to show something like a progress bar while that's happening, but we currently don't and instead it looks like nothing happened at all, until the whole file finishes uploading; that's #2684.

@gnprice gnprice changed the title iOS cannot upload video from camera roll Can't upload video from camera roll Jan 21, 2020
@gnprice gnprice added the a-compose/send Compose box, autocomplete, camera/upload, outbox, sending label Jan 21, 2020
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 23, 2020

This may depend on work in #2887, which has become stale.

If the way is clear, I can work on adding video support, as I'm familiar with both react-native-image-picker and react-native-image-crop-picker (which #2887 proposes to introduce). I know it's possible to get video from the media library and directly from the camera with some combination of these libraries — on iOS — with React Native (with a few hiccups like ivpusic/react-native-image-crop-picker#818); the details will come back to me if I start working on this, and I can see how it goes with Android.

@gnprice
Copy link
Member

gnprice commented Apr 25, 2020

I just reproduced this on iOS -- specifically on an iPhone XR running iOS 13.3 and Zulip 26.27.150.

  • I went and recorded a short video from the camera app.
  • Then went to Zulip, opened a conversation, and hit the "plus" icon and the "image" icon, to select something to upload from the camera roll.
  • The UI showed me all the photos in the camera roll, but not the video I'd just taken.

This looks like we're just not using the right API on iOS to say "please let the user select a photo or video from the camera roll" -- we're probably instead invoking the API in a way that says "please let the user select specifically a photo from the camera roll".

The relevant code in our app is in src/compose/ComposeMenu.js, specifically in handleImagePicker. It looks like this:

import * as ImagePicker from 'react-native-image-picker';
// ...

  handleImagePicker = () => {
    ImagePicker.launchImageLibrary(
      {
        quality: 1.0,
        noData: true,
        storageOptions: {
          skipBackup: true,
          path: 'images',
        },
      },
      this.handleImagePickerResponse,
    );
  };

So the fix might be as simple as changing those options we pass to that launchImageLibrary function from react-native-image-picker. In particular, from a quick look at its API docs, we should likely be passing mediaType: 'mixed'.

The next steps for someone to try to resolve this would be to

  • reproduce the issue in a development version on iOS
  • try adding that option and see if it works
  • if so, test that things still work right on Android too after that change.

@gnprice gnprice added help wanted and removed a-iOS labels Apr 25, 2020
@gnprice
Copy link
Member

gnprice commented Apr 25, 2020

We also just had another report that this didn't work on Android, with an interesting detail:

About attaching videos actually even here on android I only see images listed when we use send image option, we use the send file option to send videos

Maybe "send file" is what I used when I tried this earlier (#3624 (comment)) on Android and it worked for me? If so then that actually makes more sense -- I'd hope for this react-native-image-picker library to have the same behavior in this respect on Android vs. iOS.

Anyway, the steps I posted just above still apply, except you can probably try either Android or iOS first before testing the change on the other. Either way, be sure to test you can reproduce the issue before adding a fix 🙂

@rk-for-zulip
Copy link
Contributor

Unfortunately, mediaType: 'mixed' is not available on Android until react-native-image-picker v2.0.0; prior to that, only 'photo' and 'video' were available options on Android. (v1.1.0 docs.)

Fortunately, using it anyway it doesn't break anything – I've just tested mediaType: 'mixed' on Android for both sending an image (IconImage) and taking a picture (IconCamera) on Android, and there was no change in behavior. (That is, only images could be selected or sent.)

@chrisbobbe
Copy link
Contributor

until react-native-image-picker v2.0.0

We're now on 2.3.4 (with plans to upgrade further). Looks like the current main branch shows support for mediaType: 'mixed' on both iOS and Android, so I'd expect a fix to be pretty simple: https://github.com/react-native-image-picker/react-native-image-picker#options

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Dec 3, 2021

I'd expect a fix to be pretty simple

There's a small problem: on the current latest react-native-image-picker (4.3.0), the response from the image picker doesn't include fileName for videos, so our code fails. The fix should be pretty simple—just include that field—and I've sent a PR: react-native-image-picker/react-native-image-picker#1880 . With that change, it works as I'd expect with mediaType: 'mixed', on iOS. (Haven't tested on Android yet.)

Fixed in react-native-image-picker/react-native-image-picker@d55c53f, released in v4.4.0! 🎉

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 4, 2021
@chrisbobbe
Copy link
Contributor

I'd expect a fix to be pretty simple

I've just filed react-native-image-picker/react-native-image-picker#1881 for another issue, then maybe it'll be simple? 🤞

@Komal7209
Copy link

@chrisbobbe Is this issue assigned to someone else or still open to work because as per above conversation , I'm unable to get that is it resolved or not?

@chrisbobbe
Copy link
Contributor

This issue has gotten stalled waiting for an upstream PR (current status here); I'd recommend choosing a different issue to work on.

@Komal7209

This comment was marked as off-topic.

@alya
Copy link
Collaborator

alya commented Apr 14, 2022

@Komal7209 please see https://zulip.readthedocs.io/en/latest/overview/contributing.html#picking-an-issue-to-work-on. I highly recommend reading through that entire page as you're getting started. Thanks!

@gnprice
Copy link
Member

gnprice commented Sep 27, 2022

@RealHarshAgarwal1
Copy link

Hey everyone I'm new to open-source can anyone guide me on how I can get started I just searched good first issue and got here I also forked zulip mobile repo

@alya
Copy link
Collaborator

alya commented Oct 1, 2022

Welcome @RealHarshAgarwal1 ! Please go through the Zulip contributor guide, as well as the mobile-specific instructions at https://github.com/zulip/zulip-mobile#contributing.

@marshallmassengill
Copy link

Any update on this that can be provided? This is a major issue for us in adopting Zulip at the moment. Are there any workarounds via plugins for uploading videos?

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 23, 2023
This has changes that look interesting for zulip#3624:
  react-native-image-picker/react-native-image-picker@fcb8aa641
(More details in an upcoming commit with our fix for that issue.)

Also, a small change that should let us offer video capture on iOS:
  react-native-image-picker/react-native-image-picker@4de5e5a39
If we end up having to revert this upgrade because of something
unrelated, that change should be easy to patch in with
`patch-package`; see b546b07 for an example of using
patch-package.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 23, 2023
…13+)

This issue was stalled while waiting for react-native-image-picker
to include a filename extension in the `fileName` property of the
asset payload, specifically in the case of videos on Android. We
sent two PRs with reasonable approaches to do that, but they went
stale:
  react-native-image-picker/react-native-image-picker#1881
  react-native-image-picker/react-native-image-picker#1895
(We need appropriate filename extensions because Zulip servers want
to interpret them.)

But in January, a commit landed that actually added the filename
extensions (!):
  react-native-image-picker/react-native-image-picker@fcb8aa641
Or at least, it added the extensions in the case where the Intent
gives us a `content://` URL for the asset. (Note that the
`.getExtensionFromMimeType` function call is added to
`getFileTypeFromMime`, which is a helper for
`getAppSpecificStorageUri`, which -- starting in that same commit --
is called for videos when the URL has `content://`.)

We can guarantee getting a `content://` URL in at least the
following way: restrict this feature to Android SDK 33+. As we know
(and can confirm from reading code), the library uses Android's
preferred new media-picker UI starting at that version:
  new Intent(MediaStore.ACTION_PICK_IMAGES)
and the doc confirms that that indeed gives "content URI(s)":
  https://developer.android.com/reference/android/provider/MediaStore#ACTION_PICK_IMAGES

It'd be nicer to allow video selection on older Android too. But
that seems nontrivial, unless we can guarantee getting `content://`
URLs from the ACTION_PICK intents created by this library, and I
don't think we can. The alternative is a result we've rejected
before: a bad, generic file-picker UI that's not specialized to
images/videos, via a library codepath that uses
Intent.ACTION_GET_CONTENT.

Or I suppose I could basically rebase one of my library PRs I
mentioned at the top of this commit message, so we'd get filename
extensions irrespective of the `content://` detail. We wouldn't even
have to wait for it to land; we found `patch-package` quite usable;
see, e.g., b546b07. But at this point I think I've spent plenty of
time on this, and the benefit of the current approach will be felt
by a large and growing portion of our users.

Fixes: zulip#3624
@chrisbobbe
Copy link
Contributor

gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 18, 2023
This has changes that look interesting for zulip#3624:
  react-native-image-picker/react-native-image-picker@fcb8aa641
(More details in an upcoming commit with our fix for that issue.)

Also, a small change that should let us offer video capture on iOS:
  react-native-image-picker/react-native-image-picker@4de5e5a39
If we end up having to revert this upgrade because of something
unrelated, that change should be easy to patch in with
`patch-package`; see b546b07 for an example of using
patch-package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants