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 ]
Created attachment 262339 [details] Patch
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?
(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.
(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?
(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
Created attachment 262503 [details] Patch
(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.
(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.
So, we need platform specific test result, then
Created attachment 262505 [details] Patch
(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?
(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 on attachment 262505 [details] Patch Clearing flags on attachment: 262505 Committed r190618: <http://trac.webkit.org/changeset/190618>
All reviewed patches have been landed. Closing bug.