fix for issue #738 Some images simply do not download (Black image) -…

[mullender]

… Android

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[mullender]

fix for #738

[mullender]

This is the first part of the fix, it seems that the values here are part of the problem

[sjudd]

You will need to be more specific. What does this fix? On what device(s)?

This is a substantial decrease in performance. If it fixes specific issues on specific devices, only disable it in those particular cases.

[mullender]

Agreed.
I was making this PR late last night, usually not the best time :)
Afterwards I was thinking about how this could be related to the issue. My current thought is that there is a bug in the OS code of the MotoX 5.1: bitmap scaling is not thread safe.
When you specify the inDensity and inTargetDensity it will also trigger the same non-thread-safe code path.
I will validate this hunch by applying the same lock on this operation as is applied to the drawBitmap operation.

[TWiStErRob]

That looks like quite a significant change, I'll let @sjudd handle this one.

My view is that one it's not worth removing all that logic because it doesn't work on one or two phones out of thousands, I'd rather create an exception for those with some device detection.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[mullender]

@TWiStErRob @sjudd
I agree with both your comments and made the patch work only for the specific device that consistently has the issue.
I measured the perf impact of adding the lock, it is significant. #738 (comment)

But I think the perf impact is a good trade-off for this device, weighed against not being able to see the images at all.

[TWiStErRob]

How about?

final class BitmapDrawLock {
    // only enable the lock for Moto X 2nd gen on android 5.1
    private static final Lock INSTANCE =
            "XT1097".equals(Build.MODEL) && Build.VERSION.SDK_INT == Build.VERSION_CODES.LOLLIPOP_MR1
            ? new ReentrantLock()
            : new NoLock();

    private BitmapDrawLock() {
        // static utility class
    }

    public static Lock getInstance() {
        return INSTANCE;
    }
}

public class NoLock implements Lock {
    @Override public void lock() {
        // do nothing
    }
    @Override public void lockInterruptibly() throws InterruptedException {
        // do nothing
    }
    @Override public boolean tryLock() {
        return true;
    }
    @Override public boolean tryLock(long time, @NotNull TimeUnit unit) throws InterruptedException {
        return true;
    }
    @Override public void unlock() {
        // do nothing
    }
    @NotNull @Override public Condition newCondition() {
        throw new UnsupportedOperationException("Should not be called");
    }
}

That single INSTANCE field + getInstance can be even moved to some other class, like TransformationUtils.

[mullender]

I think your proposal fits better with the existing code in the project

[mullender]

Cleaned up the PR and implemented according to your suggestion @TWiStErRob

[TWiStErRob]

Just nitpick: direct access vs get instance in the same class. I would prefer direct access where it's defined, like in applyMatrix, and getter outside the class.

[TWiStErRob]

Thank you, man!

[mullender]

@TWiStErRob good nitpicks ;)
I updated the PR.

[sjudd]

LGTM, thanks for tracking this down and fixing it!