Closed Bug 1724808 Opened 4 months ago Closed 4 months ago

nsImageFrame::MaybeDecodeForPredictedSize() might predict the wrong decoded image size, with fission

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect

Tracking

()

RESOLVED FIXED
93 Branch
Fission Milestone MVP
Tracking Status
firefox-esr78 --- disabled
firefox-esr91 --- disabled
firefox91 --- disabled
firefox92 --- wontfix
firefox93 --- fixed

People

(Reporter: dholbert, Assigned: tnikkel)

References

Details

Attachments

(1 file)

The nsImageFrame code quoted below might not be quite right in a fission world, because GetTransformToAncestorScaleExcludingAnimated doesn't actually get our "scale to the screen", if we're an image inside of a cross-origin iframe.

Here's the code in question (note the GetTransformToAncestorScaleExcludingAnimated invocation in particular):

void nsImageFrame::MaybeDecodeForPredictedSize() {
[...]
  // OK, we're ready to decode. Compute the scale to the screen...
  mozilla::PresShell* presShell = PresContext()->PresShell();
  LayoutDeviceToScreenScale2D resolutionToScreen(
      presShell->GetCumulativeResolution() *
      nsLayoutUtils::GetTransformToAncestorScaleExcludingAnimated(this));

[...]
  // ...and use them to compute our predicted size in screen pixels.
[...]
  // Determine the optimal image size to use.
[...]
  // Request a decode.
  mImage->RequestDecodeForSize(predictedImageSize, flags);

tnikkel, do you know how much of an issue this is? The discussion/motivating-comments in bug 1151359 seem relevant here.

If we really want "screen coordinates", then we'll probably need to add some additional logic / communication here. But I'm not sure we do... Seth says in bug 1151359 comment 1, "Ideally we'd use layer coordinates", and if that's the case, then maybe we're actually fine? I suspect the parent-process-transform-agnostic sizing that we'll be getting here might be philosophically analogous to "layer coordinates"...

I suspect the real question is what size we expect to eventually paint the image at. In a scenario where we're in a transformed iframe, I'm not sure whether that painted size will match the size that we're predicting here or not. (If it will, then that's great and there's nothing to do, I suspect.)

Anyway: tnikkel, hoping you can weigh in here & close this or suggest/take action as-needed.

Flags: needinfo?(tnikkel)
Depends on: 1724904

Thanks for filing. Yes I think we will need a fix here, we have the EffectsInfo mechanism already set up to pass this info, we just need to pass the info with webrender and use it.

Unfortunately as I was working on this bug I discovered a larger issue that encompasses this, bug 1724901. I'll fix this first and then we can figure out how to fix bug 1724901.

Flags: needinfo?(tnikkel)
See Also: → 1724901
Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1e7bb4e3fb2
Take into account ancestor documents in other processes' resolution when pre-decoding images. r=hiro
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch

Setting status-firefox92=wontfix because we don't need to uplift this fix to Beta 92 for our Fission experiment.

Blocks: 1715190
Fission Milestone: --- → MVP
Depends on: 1731936
No longer depends on: 1731936
You need to log in before you can comment on or make changes to this bug.