Bug 149752 – [Cairo] fast/canvas/canvas-imageSmoothingFoo tests failed after r190383.

• Component: Platform
• Status: RESOLVED FIXED

[Hunseop Jeong]

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/5838/steps/layout-test/logs/stdio

  fast/canvas/canvas-imageSmoothingEnabled.html [ Failure ]
  fast/canvas/canvas-imageSmoothingQuality.html [ Failure ]

[Hunseop Jeong]

Created attachment 262339 [details]
Patch

[Martin Robinson]

Comment on attachment 262339 [details]
Patch

I wonder if it is possible to have Cairo more closely match the behavior of CG here? What do you think?

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 262339 [details]
> Patch
> 
> I wonder if it is possible to have Cairo more closely match the behavior of
> CG here? What do you think?

I don't think it's possible, because CG has more options than cairo, so there's always an option we can't match. In cairo Low and None are the same, which is CAIRO_FILTER_FAST, medium and default are also the same CAIRO_FILTER_GOOD and high is CAIRO_FILTER_BEST. A possible change could be:

 - None -> FAST
 - Low, Default, Medium -> GOOD
 - High -> BEST

But there are cases where Low is used for performance reasons (scrolling) and we really want to use FAST there. What we currently do in HTMLCanvasElement.h is defining a DefaultInterpolationQuality

#if USE(CG)
#define DefaultInterpolationQuality InterpolationLow
#else
#define DefaultInterpolationQuality InterpolationDefault
#endif

I think we could do the same for SmoothingQuality, and use the default value in the constructor.

[Hunseop Jeong]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 262339 [details]
> > Patch
> > 
> > I wonder if it is possible to have Cairo more closely match the behavior of
> > CG here? What do you think?
> 
> I don't think it's possible, because CG has more options than cairo, so
> there's always an option we can't match. In cairo Low and None are the same,
> which is CAIRO_FILTER_FAST, medium and default are also the same
> CAIRO_FILTER_GOOD and high is CAIRO_FILTER_BEST. A possible change could be:
> 
>  - None -> FAST
>  - Low, Default, Medium -> GOOD
>  - High -> BEST
> 
You mean, we change the match :
    
    switch (m_state->m_imageInterpolationQuality) {
    case InterpolationNone:
        cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_FAST);
        break;
    case InterpolationLow:
    case InterpolationMedium:
    case InterpolationDefault:
        cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_GOOD);
        break;
    case InterpolationHigh:
        cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_BEST);
        break;
    }
?

> But there are cases where Low is used for performance reasons (scrolling)
> and we really want to use FAST there. What we currently do in
> HTMLCanvasElement.h is defining a DefaultInterpolationQuality
> 
> #if USE(CG)
> #define DefaultInterpolationQuality InterpolationLow
> #else
> #define DefaultInterpolationQuality InterpolationDefault
> #endif
> 
> I think we could do the same for SmoothingQuality, and use the default value
> in the constructor.

and use the 'InterpolationLow' like as CG in HTMLCanvasElement?

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 262339 [details]
> > > Patch
> > > 
> > > I wonder if it is possible to have Cairo more closely match the behavior of
> > > CG here? What do you think?
> > 
> > I don't think it's possible, because CG has more options than cairo, so
> > there's always an option we can't match. In cairo Low and None are the same,
> > which is CAIRO_FILTER_FAST, medium and default are also the same
> > CAIRO_FILTER_GOOD and high is CAIRO_FILTER_BEST. A possible change could be:
> > 
> >  - None -> FAST
> >  - Low, Default, Medium -> GOOD
> >  - High -> BEST
> > 
> You mean, we change the match :
>     
>     switch (m_state->m_imageInterpolationQuality) {
>     case InterpolationNone:
>         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_FAST);
>         break;
>     case InterpolationLow:
>     case InterpolationMedium:
>     case InterpolationDefault:
>         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_GOOD);
>         break;
>     case InterpolationHigh:
>         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_BEST);
>         break;
>     }
> ?

No, as I said below, this will cause performance regression in some situations.

> > But there are cases where Low is used for performance reasons (scrolling)
> > and we really want to use FAST there. What we currently do in
> > HTMLCanvasElement.h is defining a DefaultInterpolationQuality
> > 
> > #if USE(CG)
> > #define DefaultInterpolationQuality InterpolationLow
> > #else
> > #define DefaultInterpolationQuality InterpolationDefault
> > #endif
> > 
> > I think we could do the same for SmoothingQuality, and use the default value
> > in the constructor.
> 
> and use the 'InterpolationLow' like as CG in HTMLCanvasElement?

Define a DefaultSmoothingQuality there and use it in the constructor

[Hunseop Jeong]

Created attachment 262503 [details]
Patch

[Hunseop Jeong]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 262339 [details]
> > > > Patch
> > > > 
> > > > I wonder if it is possible to have Cairo more closely match the behavior of
> > > > CG here? What do you think?
> > > 
> > > I don't think it's possible, because CG has more options than cairo, so
> > > there's always an option we can't match. In cairo Low and None are the same,
> > > which is CAIRO_FILTER_FAST, medium and default are also the same
> > > CAIRO_FILTER_GOOD and high is CAIRO_FILTER_BEST. A possible change could be:
> > > 
> > >  - None -> FAST
> > >  - Low, Default, Medium -> GOOD
> > >  - High -> BEST
> > > 
> > You mean, we change the match :
> >     
> >     switch (m_state->m_imageInterpolationQuality) {
> >     case InterpolationNone:
> >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_FAST);
> >         break;
> >     case InterpolationLow:
> >     case InterpolationMedium:
> >     case InterpolationDefault:
> >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_GOOD);
> >         break;
> >     case InterpolationHigh:
> >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_BEST);
> >         break;
> >     }
> > ?
> 
> No, as I said below, this will cause performance regression in some
> situations.
> 
> > > But there are cases where Low is used for performance reasons (scrolling)
> > > and we really want to use FAST there. What we currently do in
> > > HTMLCanvasElement.h is defining a DefaultInterpolationQuality
> > > 
> > > #if USE(CG)
> > > #define DefaultInterpolationQuality InterpolationLow
> > > #else
> > > #define DefaultInterpolationQuality InterpolationDefault
> > > #endif
> > > 
> > > I think we could do the same for SmoothingQuality, and use the default value
> > > in the constructor.
> > 
> > and use the 'InterpolationLow' like as CG in HTMLCanvasElement?
> 
> Define a DefaultSmoothingQuality there and use it in the constructor

Oh..I see. But I got a fail message after applying this patch in fast/canvas/canvas-imageSmoothingQuality.html.

PASS defaultContext.imageSmoothingQuality is 'low'
FAIL defaultContext.imageSmoothingQuality should be low. Was medium.

[Hunseop Jeong]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > Comment on attachment 262339 [details]
> > > > > Patch
> > > > > 
> > > > > I wonder if it is possible to have Cairo more closely match the behavior of
> > > > > CG here? What do you think?
> > > > 
> > > > I don't think it's possible, because CG has more options than cairo, so
> > > > there's always an option we can't match. In cairo Low and None are the same,
> > > > which is CAIRO_FILTER_FAST, medium and default are also the same
> > > > CAIRO_FILTER_GOOD and high is CAIRO_FILTER_BEST. A possible change could be:
> > > > 
> > > >  - None -> FAST
> > > >  - Low, Default, Medium -> GOOD
> > > >  - High -> BEST
> > > > 
> > > You mean, we change the match :
> > >     
> > >     switch (m_state->m_imageInterpolationQuality) {
> > >     case InterpolationNone:
> > >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_FAST);
> > >         break;
> > >     case InterpolationLow:
> > >     case InterpolationMedium:
> > >     case InterpolationDefault:
> > >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_GOOD);
> > >         break;
> > >     case InterpolationHigh:
> > >         cairo_pattern_set_filter(pattern.get(), CAIRO_FILTER_BEST);
> > >         break;
> > >     }
> > > ?
> > 
> > No, as I said below, this will cause performance regression in some
> > situations.
> > 
> > > > But there are cases where Low is used for performance reasons (scrolling)
> > > > and we really want to use FAST there. What we currently do in
> > > > HTMLCanvasElement.h is defining a DefaultInterpolationQuality
> > > > 
> > > > #if USE(CG)
> > > > #define DefaultInterpolationQuality InterpolationLow
> > > > #else
> > > > #define DefaultInterpolationQuality InterpolationDefault
> > > > #endif
> > > > 
> > > > I think we could do the same for SmoothingQuality, and use the default value
> > > > in the constructor.
> > > 
> > > and use the 'InterpolationLow' like as CG in HTMLCanvasElement?
> > 
> > Define a DefaultSmoothingQuality there and use it in the constructor
> 
> Oh..I see. But I got a fail message after applying this patch in
> fast/canvas/canvas-imageSmoothingQuality.html.
> 
> PASS defaultContext.imageSmoothingQuality is 'low'
> FAIL defaultContext.imageSmoothingQuality should be low. Was medium.

fase/canvas/canvas-imageSmoothingQuality test checks the default value of imageSmoothingQuality whether is low.

[Carlos Garcia Campos]

So, we need platform specific test result, then

[Hunseop Jeong]

Created attachment 262505 [details]
Patch

[Hunseop Jeong]

(In reply to comment #9)
> So, we need platform specific test result, then

Do I add the platform specific test expected file with FAIL result?

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #9)
> > So, we need platform specific test result, then
> 
> Do I add the platform specific test expected file with FAIL result?

Yes, that's what we expect :-) Thanks

[WebKit Commit Bot]

Comment on attachment 262505 [details]
Patch

Clearing flags on attachment: 262505

Committed r190618: <http://trac.webkit.org/changeset/190618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.