Respect setDownsampleEnabled config in DecodeProducer

[sunnylqm]

Fixes #2397

Motivation (required)

Currently setDownsampleEnabled is not properly applied if set to false.

Test Plan (required)

My understanding is that setDownsampleEnabled(false) should disable this feature no matter what other conditions are, while setDownsampleEnabled(true) may enable it or leave it to other conditions.

[facebook-github-bot]

@defHLT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[JeffGuKang]

It there any update about this?

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[sunnylqm]

ping @defHLT @oprisnik

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[pateljoel]

@defHLT @oprisnik would you mind kindly reviewing this PR?

This is a long standing and critical issue in fresco that affects React Native's image quality on Android as seen here:

#2397

facebook/react-native#32411

facebook/react-native#21301

https://snack.expo.dev/@joelpateljp/low-quality-image-android-example?platform=android

I've updated the project to a new react native version and the issue still persists.

https://snack.expo.dev/@joelpateljp/low-quality-image-android-example-2023?platform=android

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[pateljoel]

ping @defHLT @oprisnik

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[pateljoel]

gentle ping @cortinico @javache @mdvacca @yungsters @passy @oprisnik @defHLT

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@sunnylqm has updated the pull request. You must reimport the pull request before landing.

[cortinico]

@sunnylqm there is no need to update the pull request as this is a oneliner

@defHLT is the one responsible to merge this PR. Specifically he had some concerns that he mentioned internally that are preventing this PR from moving forward

[sunnylqm]

if there are confidential concerns, i don't know what else to do except update the pr😂

[cortinico]

if there are confidential concerns, i don't know what else to do except update the pr😂

FYI: Updating the PR is not helpful. @defHLT should drive this forward as I mentioned already

[pateljoel]

Thanks for the update @cortinico.

This is now news to me (and others) and the first time we have heard about something like this blocking this PR from being merged, I would say someone from the Fresco team could have sent a message on this earlier but we had to unfortunately keep pinging since this is a serious bug / issue (that is happening in prod) that needs to be merged into Fresco for a new release.

I hope it would be possible for @defHLT to provide a clearer update on this concern (if possible) and to get this PR finally merged in as this has been in limbo for over 6 years.

[pateljoel]

Apologies for asking, but has there been any progress internally about merging in this issue yet?

If the team at Facebook need a reproducible example of this issue please look at the issue below.

#2500 (comment)

[yungsters]

The primary concern with this change is the likelihood of unintended downstream impact it may have on other use cases — not employing React Native. However, this is supposedly how Fresco has always worked since 2017: 8d83ed2

A couple potential ideas for next steps:

• Figure out why downsampling produces such a poor result. It should not result in an image with lower dimensions than requested. @defHLT suggested that this method might warrant some investigation:

  fresco/imagepipeline-base/src/main/java/com/facebook/imagepipeline/transcoder/DownsampleUtil.kt

  Lines 118 to 131 in a4ac78a

  	fun ratioToSampleSize(ratio: Float): Int {
  	if (ratio > 0.5f + 0.5f * INTERVAL_ROUNDING) {
  	return 1 // should have resized
  	}
  	var sampleSize = 2
  	while (true) {
  	val intervalLength = 1.0 / (sampleSize.toDouble().pow(2.0) - sampleSize)
  	val compare = 1.0 / sampleSize + intervalLength * INTERVAL_ROUNDING
  	if (compare <= ratio) {
  	return sampleSize - 1
  	}
  	sampleSize++
  	}
  	}

• Or determine why this started regressing in React Native and identify a mitigation based on that.

[sunnylqm]

Even if it may impact other use cases, it is quite common to bump a major version with some notes about the breaking change, instead of leaving it rotting and substantially impacting react native users for years.

Do we have an option to disable downsampling regardless how smart it is? The setDownsampleEnabled simply not works, like a door you can only open but not close —— it's a bug that should be fixed in every sense.

And I don't think the downsampling performance matters here. We all know it's a tradeoff. The key point of a tradeoff is not about the price/performance, it's all about choice. Even if you can achieve 99.9% performance, I still have the right to choose a lossless way right?

[yungsters]

@sunnylqm – I understand your frustration, and I generally agree with what you're saying. (Also, I really appreciate the folks who have been pinging this and ensuring it gets the attention that it deserves.)

When I previously mentioned the following:

The primary concern with this change is the likelihood of unintended downstream impact it may have on other use cases — not employing React Native.

I was referring to other use cases internally at Meta. Like React Native, Fresco is also used in production apps (and at the scale that these apps operate at, almost anything that can go wrong often does to some degree).

Rest assured, I will continue to follow up on this internally.

[yungsters]

I just wanted to follow up here so everyone knows that this is still on my mind.

I've proposed a couple alternatives to the team, and I will report back with any updates.

[yungsters]

Thanks again for the patience. I've shared a proposal with other engineers at Meta that everyone is happy to support, and I believe it will fulfill the requirements here.

Instead of changing the existing behavior of setDownsampleEnabled(false), which is too risky of a change that is depended upon by many existing apps, I propose adding a new method and deprecating setDownsampleEnabled(boolean) entirely. The premise is that setDownsampleEnabled(boolean) is the wrong interface anyway, because setDownsampleEnabled(false) is misleading and does not actually do what most people would expect.

We can introduce a new method, setDownsampleMode(DownsampleMode), where DownsampleMode is an enum with values:

• DownsampleMode.ALWAYS behaves the same as today's setDownsampleEnabled(true).
• DownsampleMode.AUTO behaves the same as today's setDownsampleEnabled(false).
• DownsampleMode.NEVER is new and will cause Fresco to ignore the IS_RESIZING_DONE flag.

We can then deprecate setDownsampleEnabled(boolean) and refactor it to invoke setDownsampleMode for backward compatibility. The check in DecodeProducer.java that is being changed in this PR can then become:

if (mDownsampleMode != DownsampleMode.NEVER || !statusHasFlag(status, Consumer.IS_RESIZING_DONE)) {

Does this make sense and solve the problems that this PR is trying to solve? And if so, would this proposal be something that you would be willing to help implement?

[sunnylqm]

thank you for your proposal and it sounds perfect. will try to implement in a few days

[pateljoel]

@yungsters Sounds good, so if this is implemented by @sunnylqm does this mean it would be merged in quicker? Would this also get resolved for React Native as well as fresco is depended on for image loading?

[yungsters]

Yes, reviewing the new proposal will be faster. And yes, any consumers (including React Native) will benefit from the changes.

[sunnylqm]

@yungsters please review #2760