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

Update NetworkBitmapHunter.java #487

Closed
wants to merge 1 commit into from
Closed

Update NetworkBitmapHunter.java #487

wants to merge 1 commit into from

Conversation

lidemin
Copy link

@lidemin lidemin commented Apr 28, 2014

Don't know why those 2 lines codes placed there. From my understanding those 2 lines are useless and the 2nd time reset MarkableInputStream cause "can not reset" IOException.

#465

Don't know why those 2 lines codes placed there. From my understanding those 2 lines are useless and the 2nd time reset MarkableInputStream cause "can not reset" IOException.

#465
@JakeWharton
Copy link
Collaborator

This is not the appropriate course of action to fix that problem.

@lidemin
Copy link
Author

lidemin commented Apr 28, 2014

so fast. sorry leave you another comment in that issue ticket.

@lidemin
Copy link
Author

lidemin commented Apr 28, 2014

@JakeWharton So why do we need to decode that stream twice? I thought the last time should be enough.

@JakeWharton
Copy link
Collaborator

So that we can decode the metadata of the image before the actual pixel data. We need to drop MarkableInputStream in favor of something more robust to the problems of Android's bitmap decoder.

@lidemin
Copy link
Author

lidemin commented Apr 30, 2014

Hi, @JakeWharton .

Thanks for you explanation. Did not notice that option was changed.

Apparently this issue only happens for Jpeg loading with fit() or resize() being called.

Kept on tracing the codes to SkImage and JpegLib and found this:

case DSTATE_INHEADER:
    retcode = (*cinfo->inputctl->consume_input) (cinfo);
    if (retcode == JPEG_REACHED_SOS) { /* Found SOS, prepare to decompress */
      /* Set up default parameters based on header data */
      default_decompress_parms(cinfo);
      /* Set global state: ready for start_decompress */
      cinfo->global_state = DSTATE_READY;
    }
    break;

It looks like jpeg lib will read out header data until reach section SOS ( which suppose be "FFDA"). And the JPEG width&heigh info exist in section SOF0("FFC0").

In my test file [https://d13yacurqjgara.cloudfront.net/users/74571/screenshots/1526933/moriah_gradinvite_dribbble_teaser.jpg]

there are more than 1 SOF0 section but the valid one located 574438. Using Picasso to debug I found the skip position is 575308. However the SOS section located 2780.

So I guess some of the jpeg files don't not locate valid SOF0 section before SOS which cause the offset is more than 65536.

I am afraid changing MarkableInputStream will not be robust enough to adjust this kind of "wrong" format.

How about parsing JPEG file header by Picasso ? Maybe just width and height info if we just need them to fulfil the option .

Thanks again.

@JakeWharton
Copy link
Collaborator

Great investigation! We've talked about doing the header parsing for the big formats ourselves before. I've never been able to dig down into the C code to see what was actually happening.

As to MarkableInputStream and what I said above, there's nothing wrong with boundlessly buffering the downloaded dat while the decode bounds is running. We're going to end up consuming the entirety of it on a subsequent decode call. In the best case you'd buffer a small header and in the absolute worst worst worst case the whole image but then immediately decode it (no reading from a stream required).

That said, the reason we chose streaming was so that we didn't have to buffer a lot of data and instead could decode it as the data was coming in. If we do out own decoding we can buffer a lot less and retain this behavior.

@lidemin
Copy link
Author

lidemin commented Apr 30, 2014

Thanks, @JakeWharton .

Yes , agreed . Actually streaming for most of cases is good enough. And I do really think MarkableInputStream is ok.

I did use Picasso for most of my projects. Thanks for make this library. 👍 This is the first time met problem. Actually I was trying Retrofit and RxJava for Dribbble api . And there were quite a few images can not be loaded. All of them are JPEG.

@untalfranfernandez
Copy link

@lidemin I've just found the same issue with some bitmaps, actually I found that increasing MARKER constant in NetworkBitmapHunter from 65536 to 2097152 is working, did you find any other approach?

I've tried the very same picture with Ion library from Koush and it's working fine.

BTW, this is the image in case you want to have a look: http://www.asturias.com.co/administracion/imagenes/motos/usadas/Raprtor%2080%202007%20Azul.bmp

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.

3 participants