Remove GraphicsFilter

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
It should be possible to remove GraphicsFilter and replace all its uses with gfx::Filter. Though there's some weirdness that needs to be resolved first...
(Assignee)

Comment 1

3 years ago
Created attachment 8669500 [details] [diff] [review]
(part 1) - Remove BILINEAR and GAUSSIAN filter constants

These aren't used meaningfully.
Attachment #8669500 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

3 years ago
The conversions from GraphicsFilter to gfx::Filter and back (in ToFilter() and ThebesFilter()) are screwy:

- FILTER_NEAREST    <--> POINT
- FILTER_GOOD        --> GOOD   --> FILTER_BEST (not FILTER_GOOD!)
- FILTER_{FAST,BEST} --> LINEAR --> FILTER_BEST
- FILTER_FAST        --> LINEAR --> FILTER_BEST

Without knowing anything of the broader context, one way to fix it up might be the following:

- Make FILTER_GOOD <--> GOOD.

- Remove FILTER_FAST and replace it throughout with FILTER_BEST; even though
  that is conceptually weird it's what the current code effectively does.

Then there would be a 1-to-1 correspondence between GraphicsFilter and gfx::Filter and removing the former would be easy.

mattwoodrow: please tell me what I have gotten wrong about this :)
Flags: needinfo?(matt.woodrow)
Attachment #8669500 - Flags: review?(matt.woodrow) → review+
I agree, that sounds like the right plan of action.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 4

3 years ago
Created attachment 8670035 [details] [diff] [review]
(part 2) - Make gfx::FILTER::GOOD convert to GraphicsFilter::FILTER_GOOD
Attachment #8670035 - Flags: review?(matt.woodrow)
(Assignee)

Comment 5

3 years ago
Created attachment 8670039 [details] [diff] [review]
(part 3) - Remove GraphicsFilter::FILTER_FAST and replace it with FILTER_BEST

This may sound like an odd change but it's what the current code effectively
already does due to the way ToFilter() and ThebesFilter() are defined.
Attachment #8670039 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

3 years ago
Created attachment 8670045 [details] [diff] [review]
(part 4) - Replace GraphicsFilter constants with gfx::Filter equivalents

The conversion is as follows:

- GraphicsFilter::FILTER_NEAREST == gfx::Filter::POINT
- GraphicsFilter::FILTER_GOOD    == gfx::Filter::GOOD
- GraphicsFilter::FILTER_BEST    == gfx::Filter::LINEAR

Also typedef GraphicsFilter to gfx::Filter; this will be removed in the next
patch.

These changes mean ToFilter() and ThebesFilter() are no longer needed.
Attachment #8670045 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

3 years ago
(In reply to Nicholas Nethercote [:njn] from comment #6)
> Created attachment 8670045 [details] [diff] [review]
> (part 4) - Replace GraphicsFilter constants with gfx::Filter equivalents
> 

After this patch, in nsLayoutUtils::GetGraphicsFilterForFrame() we end up using Filter::LINEAR for both NS_STYLE_IMAGE_RENDERING_OPTIMIZESPEED and NS_STYLE_IMAGE_RENDERING_OPTIMIZEQUALITY.

Maybe Filter::POINT would be better for the OPTIMIZESPEED case?
(Assignee)

Comment 8

3 years ago
Created attachment 8670047 [details] [diff] [review]
(part 5) - Remove GraphicsFilter and gfxGraphicsFilter
Attachment #8670047 - Flags: review?(matt.woodrow)
Attachment #8670035 - Flags: review?(matt.woodrow) → review+
(In reply to Nicholas Nethercote [:njn] from comment #7)
> (In reply to Nicholas Nethercote [:njn] from comment #6)
> > Created attachment 8670045 [details] [diff] [review]
> > (part 4) - Replace GraphicsFilter constants with gfx::Filter equivalents
> > 
> 
> After this patch, in nsLayoutUtils::GetGraphicsFilterForFrame() we end up
> using Filter::LINEAR for both NS_STYLE_IMAGE_RENDERING_OPTIMIZESPEED and
> NS_STYLE_IMAGE_RENDERING_OPTIMIZEQUALITY.
> 
> Maybe Filter::POINT would be better for the OPTIMIZESPEED case?

That sounds like the right thing to do. It's possible that it'll change rendering and make reftests fail, will need a try run.
Attachment #8670039 - Flags: review?(matt.woodrow) → review+
Attachment #8670045 - Flags: review?(matt.woodrow) → review+
Attachment #8670047 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 10

3 years ago
Thank you for the fast reviews.
You need to log in before you can comment on or make changes to this bug.