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

[TIMOB-17567] Android: Replace the use of getFileExtensionFromUrl as it is for enco... #6216

Merged
merged 2 commits into from Nov 22, 2014

Conversation

salachi
Copy link
Contributor

@salachi salachi commented Oct 14, 2014

https://jira.appcelerator.org/browse/TIMOB-17567
Replace the use of getFileExtensionFromUrl as it is for encoded url and won't work with path with special characters.

…ncoded url and have issues with path with special characters
@hieupham007
Copy link
Contributor

Sunil, I'm not seeing any test case in the ticket for this fix. Can you update the test case please?

@salachi
Copy link
Contributor Author

salachi commented Nov 2, 2014

test case added

@ashcoding
Copy link
Contributor

I'm still getting "[INFO] : 0 . 0 . application/octet-stream" from

Ti.API.info(earthImage.image.height + ' . ' + earthImage.image.width + ' . ' + earthImage.image.mimeType);

https://github.com/appcelerator/titanium_mobile/blob/58198c641d77e17d156431666e80bae732b5c130/android/titanium/src/java/org/appcelerator/titanium/TiBlob.java#L228

Line 228 of the TiBlob, when it loads the BitmapInfo including the width and height, it actually doesn't do that cause the mimetype is "application".

changing it to:

if (mimetype == null || mimetype.startsWith("image/") || mimetype.startsWith("application/")) {

Made it work for me.

@salachi
Copy link
Contributor Author

salachi commented Nov 13, 2014

I didn't understand the comment. Why should we process mimetype 'application/' as image? what is the image filename you used?

@ashcoding
Copy link
Contributor

I used the attached zipfile "MapModule.zip" along with the "salachi:TIMOB-17567" Branch. Image was "Earth.jpg".

As mentioned in the ticket, using the line of code on android:

Ti.API.info( $.earthImage.image.height + ' . '+ $.earthImage.image.width + ' . '+ $.earthImage.image.mimeType);

Produces:

04-24 15:11:03.812: I/TiAPI(28395):  0 . 0 . application/octet-stream

The bitmap information of height an width is not loaded as the mime type as shown in the logs is "application" and not "image".

This causes the "loadBitmapInfo()" method not load the height and width resulting in the case in the logs for android.

@ashcoding
Copy link
Contributor

And I think you are right. We should only process "image". I'm trying to see why in my case I'm getting the Mime type as "application" instead.

@ashcoding
Copy link
Contributor

I found the problem here. https://github.com/appcelerator/titanium_mobile/blob/58198c641d77e17d156431666e80bae732b5c130/android/titanium/src/java/org/appcelerator/titanium/TiBlob.java#L208

Android's implementation of "URLConnection.guessContentTypeFromStream(is)" is limited. It doesn't detect JPEG.

From the source code: https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/java/net/URLConnection.java from line 695 onwards, you can see that the guess method is really really limited. It doesn't detect "image/jpeg" at all. Thus, because of that, our code returns, null and defaults to "application/octet".

We need to handle the inputstream ourselves to detect JPEG in our "guessContentTypeFromStream()" method. We can do it by doing something similar to this http://www.cnblogs.com/unixmaomao/archive/2012/12/03/2800143.html

Here's also more list of mimetypes and their binary bytes used to identify them that we could consider detecting: http://book.javanb.com/java-network-programming-3rd/javanp3-CHP-15-SECT-10.html

@salachi
Copy link
Contributor Author

salachi commented Nov 17, 2014

I tried with a jpg file and it is working fine. Here is what I tried
var picture1 = 'images/Earth.jpg';
var file1 = Ti.Filesystem.getFile(Ti.Filesystem.resourcesDirectory, picture1);
var blob1 = file1.read();
Ti.API.info('blob: ' + JSON.stringify(blob1, null, 2));

mtm.getMimeTypeFromExtension is returning the right mime type.

@ingo ingo changed the title TIMOB-17567-Replace the use of getFileExtensionFromUrl as it is for enco... [TIMOB-17567] Replace the use of getFileExtensionFromUrl as it is for enco... Nov 17, 2014
@ashcoding
Copy link
Contributor

Ah I see.. I did it with:

var earthFile, earthFileStream, earthBuffer;
function fileReader(args) {
    if (args.bytesProcessed === -1) {
        Ti.API.error('Done');
        Ti.API.info(earthBuffer.toBlob().height + ' . ' + earthBuffer.toBlob().width + ' . ' + earthBuffer.toBlob().mimeType);
        earthImage.image = earthBuffer.toBlob();
        Ti.API.info(earthImage.image.height + ' . ' + earthImage.image.width + ' . ' + earthImage.image.mimeType);
        earthBuffer.release();
        earthImage.animate({
            opacity : 1,
            duration : 100
        });
    } else {
        Ti.API.error(args.bytesProcessed + ' of ' + args.totalBytesProcessed);
        if (earthBuffer === null) {
            earthBuffer = Ti.createBuffer();
        }
        earthBuffer.append(args.buffer);
    }
}

function loadEarth() {
    earthImage.image = null;
    earthImage.opacity = 0;
    earthFile = Ti.Filesystem.getFile(Ti.Filesystem.resourcesDirectory, 'images', 'Earth.jpg');
    earthFileStream = Ti.Filesystem.openStream(Ti.Filesystem.MODE_READ, earthFile.nativePath);
    earthBuffer = null;
    Ti.Stream.pump(earthFileStream, fileReader, 512, true);
}

var window = Ti.UI.createWindow({
    backgroundColor : 'green'
});

var fishImage = Ti.UI.createImageView({
    left : '10dp',
    top : '10dp',
    right : '10dp',
    height : '200dp',
    image : '/images/ClownFish.jpg'
});
window.add(fishImage);

var earthImage = Ti.UI.createImageView({
    left : '10dp',
    bottom : '10dp',
    right : '10dp',
    height : '200dp',
    opacity : 0
});
earthImage.addEventListener('singletap', loadEarth);
window.add(earthImage);
window.open();

loadEarth(); 

@ashcoding
Copy link
Contributor

Doing it using your method did indeed produce:
1600 . 2560 . image/jpeg

Which is correct.

I think the problem comes when using Ti.Stream.pump and the loadBitmapInfo() method is used as I mentioned earlier. Could you try running my sample code and see if you can get the same "0 . 0 . application/octet-stream" as me?

@ingo ingo changed the title [TIMOB-17567] Replace the use of getFileExtensionFromUrl as it is for enco... [TIMOB-17567] Android: Replace the use of getFileExtensionFromUrl as it is for enco... Nov 18, 2014
@salachi
Copy link
Contributor Author

salachi commented Nov 20, 2014

Ashraf, you are right, there is another bug in getting mimetype from stream. Fix submitted

@ashcoding
Copy link
Contributor

Code reviewed and functionally tested. It's working correctly. :)

@hieupham007
Copy link
Contributor

Code reviewed. Looks good

hieupham007 added a commit that referenced this pull request Nov 22, 2014
[TIMOB-17567] Android: Replace the use of getFileExtensionFromUrl as it is for enco...
@hieupham007 hieupham007 merged commit 0c5c10f into tidev:master Nov 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants