Loading media is isolated, complex to use.

[rusmichal]

Description

I'm reviewing your SDK and I've noticed that your API doesn't help in order to get the media. OK It does but doesn't allow to use any additional tools which can manage any files stored on the device. Creating chat application with Twilio I'm gonna create attachment feature and allows user to send images.

I use Glide for image lazy loading. Your SDK provide method download(OutputStream, StatusListener,ProgressListener). There is no way to tell SDK: "Hey you have image loader, take that one and load image with it". Giving download method with SDK you force developers to use any OutputStream (File, Byte etc) to store image with it. But you doesn't provide anything that easily will manage these outputs because these data will take place in memory or storage. That means when I'm going to create the feature and thousand of people will use it then I have two ways:

1. Let the devices blow up.
2. Reinvent the wheel again and create image lazy loading just for Twilio images.

As you see your approach is not flexible. :(

Steps to Reproduce

See the code.

Code (optional)

public final class Media {
        protected Media() {
        }

        public String getSid() {
            Message.this.checkDisposed("Media.getSid");
            return this.nativeGetSid(Message.this);
        }

        public String getFileName() {
            Message.this.checkDisposed("Media.getFileName");
            return this.nativeGetFileName(Message.this);
        }

        public String getType() {
            Message.this.checkDisposed("Media.getType");
            return this.nativeGetType(Message.this);
        }

        public long getSize() {
            Message.this.checkDisposed("Media.getSize");
            return this.nativeGetSize(Message.this);
        }

        public void download(OutputStream stream, StatusListener status, ProgressListener progress) {
            Message.this.checkDisposed("Media.download");
            if (stream == null) {
                throw new IllegalArgumentException("Stream cannot be null in Media.download()");
            } else {
                this.nativeDownload(Message.this, stream, new StatusListenerForwarder(status), new ProgressListenerForwarder(progress));
            }
        }

        private native String nativeGetSid(Message var1);

        private native String nativeGetFileName(Message var1);

        private native String nativeGetType(Message var1);

        private native long nativeGetSize(Message var1);

        private native void nativeDownload(Message var1, OutputStream var2, StatusListener var3, ProgressListener var4);
    }

Expected Behavior

It would be nice if I have the url image in Media model. But probably security issues. So it would be nice to have INPUTSTREAM and get the data from that input.

Actual Behavior

I have to provide OutputStream and pray that these image will fit in memory. In other words. If I provide OutputStream I have no clue what will happen here. If the data will be huge I will got OOM.

Reproduces how Often

[What percentage of the time does it reproduce?]

Logs

Please collect logs as described here. Full unedited logs help reproduce and fix issues faster.

// Log output when issue occurs

Or attach it as a file.

Versions

All relevant version information for issue.

Chat Android SDK

3.1.1

Android API

23

Android Device

Huawei P9

[rusmichal]

any ideas, roadmap how we can improve it, i can help you :)

[berkus]

Hi, the media is still in beta, thank you for your feedback - we will try to incorporate some ideas from your feedback in the future releases. If you have more ideas - don't hesitate to share :)

[rusmichal]

Sure, I used Glide and I followed these guide: https://bumptech.github.io/glide/tut/custom-modelloader.html#writing-the-modelloader in order to tell Glide how he can handle images from Twilio.

I've created my own ChatService which handles Twilio APIs from SDK like subscribing to channels, send messages, retrieving data channels etc. It is just a wrapper. In order to extend that service I've created TwilioImageDownloader. The implementation looks like that:

override fun download(channelSidOrId: String, messageSid: Long): Single<InputStream> {
        return /* some code here */
                    Single.create<InputStream> {
                        channel.messages.getMessageByIndex(messageSid, object : CallbackListener<Message>() {
                            override fun onSuccess(message: Message) {
                                var outStream: ByteArrayOutputStream? = ByteArrayOutputStream(message.media.size.toInt())

                                message.media.download(outStream, object : StatusListener() {
                                    override fun onSuccess() {}

                                }, object : ProgressListener() {
                                    override fun onStarted() {}

                                    override fun onProgress(p0: Long) {}

                                    override fun onCompleted(p0: String?) {
                                        val inputStream = ByteArrayInputStream(outStream?.toByteArray())
                                        outStream = null
                                        it.onSuccess(inputStream)
                                    }
                                })
                            }

                            override fun onError(errorInfo: ErrorInfo?) {
                                it.onError(Throwable(errorInfo.toString()))
                            }
                        })
                    }
                }
    }

So I use download method from Media, convert OutputStream into InputStream and pass it. I want to highlighted that here is a bottleneck. I mean transform OutputStream to InputStream which is more friendly for Glide. But here it is huge memory consumption, because method outStream?.toByteArray() copies byte array of OutputStream and pass it to InputStream.

From my point of view. The media model shouldn't be responsible for downloading own resources. It is a model. He shouldn't care about the other resources needed somewhere else. As a model just should keep the data that describe that model.

Glide configuration:

class TwilioModelLoader(private val twilioImageDownloader: TwilioImageDownloader) : ModelLoader<String, InputStream> {

    override fun buildLoadData(model: String, width: Int, height: Int, options: Options): ModelLoader.LoadData<InputStream>? =
            ModelLoader.LoadData<InputStream>(ObjectKey(model), TwilioDataFetcher(model, twilioImageDownloader))

    override fun handles(model: String): Boolean = model.startsWith(SCHEME)

    companion object {
        const val SCHEME = "twilio://"
    }
}

Because the download method from Media has channelSid and messageIndex as the params. I assumed that I have to build specific model for Glide and ask Glide with that model in order to get the image. This model will be kind of the key for the cache. To make the model more specific and avoid accidentally loading invalid model I use prefix for that model. So with all these things I've created Twilio model which is simple url and Glide use it like that:

Glide.with(context).load("twilio://$channelSid:$messageIndex").into(imageView)

If I passed "twilio://$channelSid:$messageIndex" to Glide, he can recognize the URL because of prefix and can load the image from cache, or start getting the image from Remote.

TwilioDataFetcher gathering the image.

class TwilioDataFetcher(private val model: String,
                        private val twilioImageDownloader: TwilioImageDownloader) : DataFetcher<InputStream> {

    private val compositeDisposable = CompositeDisposable()

    override fun getDataClass(): Class<InputStream> = InputStream::class.java

    override fun cleanup() {
        compositeDisposable.clear()
    }

    override fun getDataSource(): DataSource = DataSource.REMOTE

    override fun cancel() {
        compositeDisposable.clear()
    }

    override fun loadData(priority: Priority, callback: DataFetcher.DataCallback<in InputStream>) {
        val (channelId, messageIndex) = model.substring(TwilioModelLoader.SCHEME.length).split(":")

        compositeDisposable.addAll(twilioImageDownloader.download(channelId, messageIndex.toLong()).subscribe({
            callback.onDataReady(it)
        }, {
            callback.onLoadFailed(TwilioImageException(it))
        }))
    }
}

I pass the TwilioImageDownloader and the model. Once Glide can't load the image from cache the loadData method is executed.

There are two important things. First of all, get the data required to download method from model. As I mentioned before: This is model"twilio://$channelSid:$messageIndex"

val (channelId, messageIndex) = model.substring(TwilioModelLoader.SCHEME.length).split(":") I'm splitting the model into channelSid and messageIndex with this line. I call TwilioImageDownloader::download method with these data. As I mentioned earlier, it emits InputStream (converted from OutputStream) and Glide is taking care of caching it.

[rusmichal]

it is worth to think about thumbnail. You allowed to send files with 150 MB limitation. So If I build the chat app then it will be deadly to display such huge images (5MB or even more). So as an developer I should be able to get thumbnail or full size image. I can choose depends on app flow which size should be loaded.

[berkus]

@rusmichal Thank you, I noted down your comments and will see how that aligns with our plans. Will post here with new findings. Thanks again!

[berkus]

Hi!

I will close this one, there are changes coming with the next version of Media, which should address these issues. I'm sure you'll see it in the changelog when the changes are released :)

[rusmichal]

@berkus I don't see any changes related to Media in a few last releases

[rusmichal]

bump

[berkus]

@rusmichal Yes, this is currently lower priority compared to other work we're doing.

We haven't forgotten about it and will expose the download URL for you to use.