Cleanup proposal for .fromBytes().load(byte[])

[TWiStErRob]

Glide Version: 3.5+ -> 4.0
Issue details: StreamByteArrayLoader constructor is deprecated and will be removed along with .load(byte[], id) method so it won't be possible to pass the id to ByteArrayFetcher without rewriting the removed deprecated code. So the ByteArrayFetcher constructor should be deprecated too in 3.6.0 and be removed in 4.0 along with the other proposed changes in this issue.
The documentation fo ByteArrayFetcher says:

Requires an id to be passed in to identify the data in the byte array because there is no cheap/simple way to obtain a useful id from the data itself.

which was probably true pre-.signature(), but that statement is not needed any more. Currently all byte[]s are treated equal, because the fetcher doesn't provide a sensible id (default is "") in Engine.load. Which means .fromBytes() and .load(byte[]) can't be used with memory cache and also requires a signature to be passed in (I guess this is the reason why those two things are inside fromBytes).

To reach consistency with other methods I suggest:

ByteArrayFetcher.getId() { return bytes.toString(); }

This will have the following effects:

• different arrays will be treated as different data
  equivalent to file path or unique uri
• the ByteArrayFetcher.String id field will be clearly useless, as should be, based on the .signature() API
  (other Fetchers also have their model as getId()
• .signature(new StringSignature(UUID.randomUUID().toString())) could be removed from RequestManager.fromBytes() since the byte[]'s memory address will uniqely identify the data
  all other uses of signature in RequestManager is either a deprecation workaround or sensible default (ApplicationVersionSignature)
• .skipMemoryCache(true /*skipMemoryCache*/) may be removed from RequestManager.fromBytes(), because there may be transformations and other operations like creating bitmaps, which are worth caching. It's currently not possible to say fromBytes().skipMemoryCache(false) and get valid results.
• Anyone loading a byte[] then modifiying the contents and loading it again need to provide a signature, but for most cases I see that won't be the case. And the cache gains are more useful.

@sjudd Please let me know what you think about the idea.

[sjudd]

A couple of things:

1. I don't think toString() is safe enough to use as identity? There aren't any guarantees that different byte arrays with different contents will output different values from toString()?
2. There isn't any point in caching these things typically, because the bytes are what are ultimately cached, not the intermediate Bitmap. I don't think people typically want to load a byte array from an image repeatedly?

[TWiStErRob]

Currenlty ALL byte arrays are treated as equal, regardless of length and memory address, let alone contents. byte[].toString() is not new String(bytes), but [B@cafebabe It doesn't have to be exactly toString, using Integer.toHexString(System.identityHashCode()) is just as good and that has to be unique.

Did you by any chance confuse fromBytes() with asBytes(), I'm asking because the "bytes are what are ultimately cached" part doesn't make sense to me?

Imagine displaying a byte[] with a resize, a grayscale, a blur and a padding+shadow transformation. Would you still say it's not worth caching the result then?

If the same byte[] is passed in I would expect the same Bitmap to come out without going through the full Glide pipeline. Repeated load may happen if you have a List<Item> in an adapter with a byte[] Item.image field.

This issue came from a few hours of debugging why a particular part of my app wasn't working. The effects I lined up were assumptions I had, based on previous experience with other .load(X) methods and stuff just didn't work. :) I was using using().from(byte[].class).

[sjudd]

Ah sorry yes I did confuse the two.

I think the goal was to try to encourage people to actually provide some meaningful id, since the only way we could actually cache the byte array is to both rely on the contents of the array never changing and to rely on the user to provide the same byte array instance for each subsequent load.

I agree it's worth caching, I just don't think there's a great way for us to come up with a reasonable key. In most cases, users should be able to do so (where did they get the bytes from?), and can by just passing in an appropriate signature?

The problem with putting anything in the id field is that users can no longer tell us the key should be. If they provide a signature, they still won't get a cache hit because they key will still be partially dependent on the byte array, rather than the id they provide.

[TWiStErRob]

But the path to provide meaningful ID will disappear by removing the .load(byte[], String) overload. Setting a default id, but not having a way to replace it, that's really an issue I didn't think of, what if two different byte[] are the same from the user's perspective, there's no way to override then.

On the other hand everything else works as byte[] is not, currently.
Think about a File, whose contents can change, but the path didn't. Think about a content Uri, where openFile could actually return anything without the Uri changing. It's the same as providing the same byte[] with different bytes at the same positions. Do you consider File and Uri broken? No, that's why the .signature() was introduced, so users have control of cache invalidation.

I think the goal was to try to encourage people to actually provide some meaningful id.

I think this could be achieved if we remove both of those lines from fromBytes, because otherwise they just work because it is reloaded EVERY time. But, if the same image showed up for every byte[] then they would probably check the docs and see that you need a signature for byte[], which could be even enforced by returning a class from load(byte[]) that only has a signature method :)

[sjudd]

I think there are three choices:

1. Include a default id and no signature
2. Include no default id and but include a default signature
3. Include no default id and no default signature

Each of these has it's own problem:

1. It's not possible to replace the cache key if you do have some better way of identifying the bytes.
2. Different byte arrays end up with different cache keys, but the same byte array containing the same data won't get the same cache key.
3. All byte arrays end up with the same cache key.

All of these error cases are relatively subtle too. Is there any way we can either eliminate one of the problems above, or make one of the failure cases more obvious?

[TWiStErRob]

1. I think we can eliminate this solution altogether
   because there would be no workaround for the problem of cache keys.
2. This is the same as 3. + skipMemoryCache + DiskCacheStrategy.NONE
3. This would load the same image all the time, but could be made more obvious (see below).

How about going with 3. in this way:

ByteArrayFetcher.getId() {
     return "";
}
/** add: You need to set a signature or disable all caching explicitly. */
RequestManager.fromBytes() {
    return (DrawableTypeRequest<byte[]>) loadGeneric(byte[].class)
        // remove: .signature(new StringSignature(UUID.randomUUID().toString()))
        // remove: .diskCacheStrategy(DiskCacheStrategy.NONE)
        // remove: .skipMemoryCache(true /*skipMemoryCache*/);
}
GenericRequest.init(...) {
    // ...
    if (model != null) {
        // add: check for byte[] and require user to chose how to handle them
        if (model instanceof byte[] && (isMemoryCacheable || diskCacheStrategy != NONE))
                check("signature", signature == EmptySignature.obtain()? null : signature,
                        "try .signature(Key) or .skipMemoryCache(true).diskCacheStrategy(NONE)");
        }
        // ... existing stuff
    }
}

Would make sure the there's a signature, or forces the user to explicitly disable the diskCache and the memory cache. It means that load(byte[]) would throw by default, but we can consider it the same amount advanced as using is, which takes few rounds to get right usually. An exception would make them think about it at least, and it seems these messages are well-defining how to solve the problems because I don't remember any issues related to these since #101.