Closed Bug 1063129 Opened 5 years ago Closed 5 years ago

imgFrame uses BGRA instead of BGRX for opaque surface (JPG)

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed

People

(Reporter: BenWa, Assigned: mwu)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file gdb trace
I have an imgFrame with mFormat mozilla::gfx::B8G8R8X8 but it has an mOptSurface with format mozilla::gfx::B8G8R8A8. This is forcing us to blend an opaque image.
Assignee: nobody → mwu
Attachment #8484520 - Flags: review?(seth)
Comment on attachment 8484520 [details] [diff] [review]
Switch to mImageSurface to RGBX on SetHasNoAlpha

Review of attachment 8484520 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgFrame.cpp
@@ +739,5 @@
>  {
>    if (mFormat == SurfaceFormat::B8G8R8A8) {
>      mFormat = SurfaceFormat::B8G8R8X8;
> +    MOZ_ASSERT(mImageSurface);
> +    RefPtr<DataSourceSurface> surf = CreateLockedSurface(mVBuf, mSize, mFormat);

Why do you have this local here? If you're trying to get a specific lifetime this should be documented.
Actually, we shouldn't need that local now that I think about it. The code should do the right thing.
Attachment #8484520 - Attachment is obsolete: true
Attachment #8484520 - Flags: review?(seth)
Attachment #8484530 - Flags: review?(seth)
Verified the fix. Both mOptImage and mImageSurface have the right format when ::Optimize() is called.
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

Review of attachment 8484530 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!

::: image/src/imgFrame.cpp
@@ +739,5 @@
>  {
>    if (mFormat == SurfaceFormat::B8G8R8A8) {
>      mFormat = SurfaceFormat::B8G8R8X8;
> +    MOZ_ASSERT(mImageSurface);
> +    mImageSurface = CreateLockedSurface(mVBuf, mSize, mFormat);

So it looks to me like the root cause is that OptimizeSourceSurface() always returns a surface with alpha if you pass it a surface with alpha, right?

That makes sense, but just from looking at the code in SetHasNoAlpha() I wasn't really clear on what was going on here. Please add a brief comment explaining, so people reading this code in the future will understand.
Attachment #8484530 - Flags: review?(seth) → review+
Yeah - a lot of OptimizeSourceSurface implementations actually just return the same pointer.

This also optimizes rendering while the image is loading though - it's worth converting our SourceSurface wrapper as soon as we discover the alpha channel isn't needed.
(In reply to Seth Fowler [:seth] from comment #6)
> So it looks to me like the root cause is that OptimizeSourceSurface() always
> returns a surface with alpha if you pass it a surface with alpha, right?
> 

Sorry, I misread this question.

That's not the root cause. The surface we pass into OptimizeSourceSurface is a RGBA surface rather than RGBX so it can't know to optimize it to RGBX unless it reads every pixel. Instead we have to create a new DataSourceSurface so it's a SurfaceFormat::B8G8R8X8.

I think the only non-obvious thing here is that SetHasNoAlpha() is always called from the encoder, so there's always a mImageSurface to update since the imgFrame is locked.
I think part of the confusion is the current comment on SetHasNoAlpha - it makes it sound like this has something to do with Optimize(), but that's not the case anymore. I'll reword it.
https://hg.mozilla.org/mozilla-central/rev/cc9e31b9256e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1090988
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

Approval Request Comment
[Feature/regressing bug #]: Regressed by bug 994081
[User impact if declined]: Opaque images render more slowly.
[Describe test coverage new/current, TBPL]: This one is difficult to test since the output is the same with or without. However, it has baked for a while now.
[Risks and why]: Low - I can't think of any issues.
[String/UUID change made/needed]: None
Attachment #8484530 - Flags: approval-mozilla-beta?
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

(In reply to Michael Wu [:mwu] from comment #12)
> Approval Request Comment
> [Feature/regressing bug #]: Regressed by bug 994081
Bug 994081 was fixed in 32 and this bug was fixed in 35. Can you please clarify what you mean by "regressed by"?

> [User impact if declined]: Opaque images render more slowly.
> [Describe test coverage new/current, TBPL]: This one is difficult to test
> since the output is the same with or without. However, it has baked for a
> while now.

If this is a small perf improvement, even though it is a small code change I don't think we should take it now that we've already cut beta9.

I'm marking this as beta-. If I have missed something in my review of this bug and you think this should in fact still be taken in 34, please renom and provide additional context/justification.
Flags: needinfo?(mwu)
Attachment #8484530 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

This is regressing us on some benchmarks that our partners are using internally.
Flags: needinfo?(mwu)
Attachment #8484530 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

OK. This is a simple enough fix that we can take it in 34 beta10. Beta+
Attachment #8484530 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8484530 [details] [diff] [review]
Switch mImageSurface to RGBX on SetHasNoAlpha, v2

Given that this has been backed out, is not a must fix for 34, and has now missed beta10, let's defer this fix to 35, where it has already landed and stuck.
Attachment #8484530 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Clearing NI as there's nothing to do - fix is deferred to 35.
Flags: needinfo?(mwu)
You need to log in before you can comment on or make changes to this bug.