Bug 149752 - [Cairo] fast/canvas/canvas-imageSmoothingFoo tests failed after r190383.
Summary: [Cairo] fast/canvas/canvas-imageSmoothingFoo tests failed after r190383.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hunseop Jeong
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-02 09:35 PDT by Hunseop Jeong
Modified: 2015-10-06 09:35 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2015-10-02 09:41 PDT, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2015-10-05 23:01 PDT, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (6.28 KB, patch)
2015-10-05 23:49 PDT, Hunseop Jeong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hunseop Jeong 2015-10-02 09:35:51 PDT
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 ]
Comment 1 Hunseop Jeong 2015-10-02 09:41:35 PDT
Created attachment 262339 [details]
Patch
Comment 2 Martin Robinson 2015-10-04 23:42:41 PDT
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?
Comment 3 Carlos Garcia Campos 2015-10-05 00:13:35 PDT
(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.
Comment 4 Hunseop Jeong 2015-10-05 19:41:38 PDT
(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?
Comment 5 Carlos Garcia Campos 2015-10-05 22:50:09 PDT
(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
Comment 6 Hunseop Jeong 2015-10-05 23:01:42 PDT
Created attachment 262503 [details]
Patch
Comment 7 Hunseop Jeong 2015-10-05 23:04:08 PDT
(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.
Comment 8 Hunseop Jeong 2015-10-05 23:07:23 PDT
(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.
Comment 9 Carlos Garcia Campos 2015-10-05 23:13:36 PDT
So, we need platform specific test result, then
Comment 10 Hunseop Jeong 2015-10-05 23:49:05 PDT
Created attachment 262505 [details]
Patch
Comment 11 Hunseop Jeong 2015-10-05 23:51:11 PDT
(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?
Comment 12 Carlos Garcia Campos 2015-10-06 00:44:43 PDT
(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
Comment 13 WebKit Commit Bot 2015-10-06 09:35:32 PDT
Comment on attachment 262505 [details]
Patch

Clearing flags on attachment: 262505

Committed r190618: <http://trac.webkit.org/changeset/190618>
Comment 14 WebKit Commit Bot 2015-10-06 09:35:37 PDT
All reviewed patches have been landed.  Closing bug.