[Spice-devel] [PATCH 08/11] spicec: add sw canvas diff check
Marc-André Lureau
mlureau at redhat.com
Tue Sep 24 07:07:53 PDT 2013
----- Original Message -----
> On 09/24/2013 02:42 PM, Marc-André Lureau wrote:
>
>
> Looks good, but can you explain why you disable diff check for fill, text?
Setting CHECKDIFF=1 environment variable will compare the rendering of the selected canvas with a software canvas. This is useful for debugging some rendering issues, however it is far from being perfect, since it's not able to tell whether one or the other is actually faulty. It's a strong indication though of which operations are incorrect.
Ideally, all operations should be checked, however, a few of them are disabled by default because they fail all the time, and it looks like pixman is not very accurate for blending/compositing (at least visually, it seems gl has better rendering)
> > ---
> > client/display_channel.cpp | 88
> > +++++++++++++++++++++++++++++++++-------------
> > client/display_channel.h | 2 ++
> > client/red_pixmap.h | 19 ++++++++++
> > client/utils.h | 3 ++
> > 4 files changed, 88 insertions(+), 24 deletions(-)
> >
> > diff --git a/client/display_channel.cpp b/client/display_channel.cpp
> > index 49a4c6a..c9ce3d8 100644
> > --- a/client/display_channel.cpp
> > +++ b/client/display_channel.cpp
> > @@ -611,6 +611,7 @@ DisplayChannel::DisplayChannel(RedClient& client,
> > uint32_t id,
> > #endif
> > , _interrupt_update (*this)
> > , _mig_wait_primary (false)
> > + , _check_diff (getenv("CHECKDIFF") != NULL)
> > {
> > DisplayHandler* handler =
> > static_cast<DisplayHandler*>(get_message_handler());
> >
> > @@ -1276,6 +1277,22 @@ void DisplayChannel::create_canvas(int surface_id,
> > const std::vector<int>& canva
> > if (i == canvas_types.size()) {
> > THROW("create canvas failed");
> > }
> > +
> > + try {
> > + if (!_check_diff)
> > + return;
> > +
> > + SCanvas *canvas = new SCanvas(surface_id == 0, width, height,
> > format,
> > + screen()->get_window(),
> > + _pixmap_cache, _palette_cache,
> > _glz_window,
> > + _surfaces_cache);
> > + _swsurfaces_cache[surface_id] = canvas;
> > + if (surface_id == 0) {
> > + LOG_INFO("display %d: check sw", get_id());
> > + }
> > + } catch (...) {
> > + spice_warn_if_reached();
> > + }
> > }
> >
> > void DisplayChannel::handle_mode(RedPeer::InMessage* message)
> > @@ -1620,29 +1637,52 @@ void
> > DisplayChannel::handle_surface_destroy(RedPeer::InMessage* message)
> > }
> > }
> >
> > -#define PRE_DRAW
> > -#define POST_DRAW
> > +#define DRAW(type, can_diff) { \
> > + canvas->draw_##type(*type, message->size()); \
> > + if (type->base.surface_id == 0) { \
> > + invalidate(type->base.box, false); \
> > + } \
> > + if ((can_diff) && _check_diff) { \
> > + Canvas *swcanvas = _swsurfaces_cache[type->base.surface_id]; \
> > + swcanvas->draw_##type(*type, message->size()); \
> > + check_diff(swcanvas, canvas, type->base.box); \
> > + } \
> > +}
> > +
> > +#include "red_pixmap_sw.h"
> > +
> > +void check_diff(Canvas *a, Canvas *b, const SpiceRect& rect)
> > +{
> > + QRegion region;
> > + RedPixmapSw *ap = new RedPixmapSw(rect.right, rect.bottom,
> > RedDrawable::RGB32, true, 0);
> > + RedPixmapSw *bp = new RedPixmapSw(rect.right, rect.bottom,
> > RedDrawable::RGB32, true, 0);
> > +
> > + region_init(®ion);
> > + region_add(®ion, &rect);
> > + a->copy_pixels(region, *ap);
> > + b->copy_pixels(region, *bp);
> > + ap->equal(*bp, rect);
> >
> > -#define DRAW(type) { \
> > - PRE_DRAW; \
> > - canvas->draw_##type(*type, message->size()); \
> > - POST_DRAW; \
> > - if (type->base.surface_id == 0) { \
> > - invalidate(type->base.box, false); \
> > - } \
> > + delete ap;
> > + delete bp;
> > }
> >
> > void DisplayChannel::handle_copy_bits(RedPeer::InMessage* message)
> > {
> > Canvas *canvas;
> > SpiceMsgDisplayCopyBits* copy_bits =
> > (SpiceMsgDisplayCopyBits*)message->data();
> > - PRE_DRAW;
> > canvas = _surfaces_cache[copy_bits->base.surface_id];
> > + Canvas *swcanvas = _swsurfaces_cache[copy_bits->base.surface_id];
> > +
> > canvas->copy_bits(*copy_bits, message->size());
> > - POST_DRAW;
> > if (copy_bits->base.surface_id == 0) {
> > invalidate(copy_bits->base.box, false);
> > }
> > +
> > + if (_check_diff) {
> > + swcanvas->copy_bits(*copy_bits, message->size());
> > + check_diff(swcanvas, canvas, copy_bits->base.box);
> > + }
> > }
> >
> > void DisplayChannel::handle_draw_fill(RedPeer::InMessage* message)
> > @@ -1650,7 +1690,7 @@ void
> > DisplayChannel::handle_draw_fill(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawFill* fill =
> > (SpiceMsgDisplayDrawFill*)message->data();
> > canvas = _surfaces_cache[fill->base.surface_id];
> > - DRAW(fill);
> > + DRAW(fill, FALSE);
> > }
> >
> > void DisplayChannel::handle_draw_opaque(RedPeer::InMessage* message)
> > @@ -1658,7 +1698,7 @@ void
> > DisplayChannel::handle_draw_opaque(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawOpaque* opaque =
> > (SpiceMsgDisplayDrawOpaque*)message->data();
> > canvas = _surfaces_cache[opaque->base.surface_id];
> > - DRAW(opaque);
> > + DRAW(opaque, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_copy(RedPeer::InMessage* message)
> > @@ -1666,7 +1706,7 @@ void
> > DisplayChannel::handle_draw_copy(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawCopy* copy =
> > (SpiceMsgDisplayDrawCopy*)message->data();
> > canvas = _surfaces_cache[copy->base.surface_id];
> > - DRAW(copy);
> > + DRAW(copy, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_blend(RedPeer::InMessage* message)
> > @@ -1674,7 +1714,7 @@ void
> > DisplayChannel::handle_draw_blend(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawBlend* blend =
> > (SpiceMsgDisplayDrawBlend*)message->data();
> > canvas = _surfaces_cache[blend->base.surface_id];
> > - DRAW(blend);
> > + DRAW(blend, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_blackness(RedPeer::InMessage* message)
> > @@ -1682,7 +1722,7 @@ void
> > DisplayChannel::handle_draw_blackness(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawBlackness* blackness =
> > (SpiceMsgDisplayDrawBlackness*)message->data();
> > canvas = _surfaces_cache[blackness->base.surface_id];
> > - DRAW(blackness);
> > + DRAW(blackness, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_whiteness(RedPeer::InMessage* message)
> > @@ -1690,7 +1730,7 @@ void
> > DisplayChannel::handle_draw_whiteness(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawWhiteness* whiteness =
> > (SpiceMsgDisplayDrawWhiteness*)message->data();
> > canvas = _surfaces_cache[whiteness->base.surface_id];
> > - DRAW(whiteness);
> > + DRAW(whiteness, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_invers(RedPeer::InMessage* message)
> > @@ -1698,7 +1738,7 @@ void
> > DisplayChannel::handle_draw_invers(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawInvers* invers =
> > (SpiceMsgDisplayDrawInvers*)message->data();
> > canvas = _surfaces_cache[invers->base.surface_id];
> > - DRAW(invers);
> > + DRAW(invers, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_rop3(RedPeer::InMessage* message)
> > @@ -1706,7 +1746,7 @@ void
> > DisplayChannel::handle_draw_rop3(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawRop3* rop3 =
> > (SpiceMsgDisplayDrawRop3*)message->data();
> > canvas = _surfaces_cache[rop3->base.surface_id];
> > - DRAW(rop3);
> > + DRAW(rop3, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_stroke(RedPeer::InMessage* message)
> > @@ -1714,7 +1754,7 @@ void
> > DisplayChannel::handle_draw_stroke(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawStroke* stroke =
> > (SpiceMsgDisplayDrawStroke*)message->data();
> > canvas = _surfaces_cache[stroke->base.surface_id];
> > - DRAW(stroke);
> > + DRAW(stroke, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_text(RedPeer::InMessage* message)
> > @@ -1722,7 +1762,7 @@ void
> > DisplayChannel::handle_draw_text(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawText* text =
> > (SpiceMsgDisplayDrawText*)message->data();
> > canvas = _surfaces_cache[text->base.surface_id];
> > - DRAW(text);
> > + DRAW(text, FALSE);
> > }
> >
> > void DisplayChannel::handle_draw_transparent(RedPeer::InMessage* message)
> > @@ -1730,7 +1770,7 @@ void
> > DisplayChannel::handle_draw_transparent(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawTransparent* transparent =
> > (SpiceMsgDisplayDrawTransparent*)message->data();
> > canvas = _surfaces_cache[transparent->base.surface_id];
> > - DRAW(transparent);
> > + DRAW(transparent, TRUE);
> > }
> >
> > void DisplayChannel::handle_draw_alpha_blend(RedPeer::InMessage* message)
> > @@ -1738,7 +1778,7 @@ void
> > DisplayChannel::handle_draw_alpha_blend(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawAlphaBlend* alpha_blend =
> > (SpiceMsgDisplayDrawAlphaBlend*)message->data();
> > canvas = _surfaces_cache[alpha_blend->base.surface_id];
> > - DRAW(alpha_blend);
> > + DRAW(alpha_blend, FALSE);
> > }
> >
> > void DisplayChannel::handle_draw_composite(RedPeer::InMessage* message)
> > @@ -1746,7 +1786,7 @@ void
> > DisplayChannel::handle_draw_composite(RedPeer::InMessage* message)
> > Canvas *canvas;
> > SpiceMsgDisplayDrawComposite* composite =
> > (SpiceMsgDisplayDrawComposite*)message->data();
> > canvas = _surfaces_cache[composite->base.surface_id];
> > - DRAW(composite);
> > + DRAW(composite, TRUE);
> > }
> >
> > void DisplayChannel::streams_time()
> > diff --git a/client/display_channel.h b/client/display_channel.h
> > index 197ae73..8a30289 100644
> > --- a/client/display_channel.h
> > +++ b/client/display_channel.h
> > @@ -187,6 +187,7 @@ private:
> >
> > private:
> > SurfacesCache _surfaces_cache;
> > + SurfacesCache _swsurfaces_cache;
> > PixmapCache& _pixmap_cache;
> > PaletteCache _palette_cache;
> > GlzDecoderWindow& _glz_window;
> > @@ -226,6 +227,7 @@ private:
> > InterruptUpdate _interrupt_update;
> >
> > bool _mig_wait_primary;
> > + bool _check_diff;
> > friend class SetModeEvent;
> > friend class CreatePrimarySurfaceEvent;
> > friend class DestroyPrimarySurfaceEvent;
> > diff --git a/client/red_pixmap.h b/client/red_pixmap.h
> > index 240ed76..3f12855 100644
> > --- a/client/red_pixmap.h
> > +++ b/client/red_pixmap.h
> > @@ -36,6 +36,25 @@ public:
> > bool is_big_endian_bits();
> > virtual RedDrawable::Format get_format() { return _format; }
> >
> > + bool equal(const RedPixmap &other, const SpiceRect &rect) const {
> > + spice_debug("l:%d r:%d t:%d b:%d", rect.left, rect.right,
> > rect.top, rect.bottom);
> > + for (int x = rect.left; x < rect.right; ++x)
> > + for (int y = rect.top; y < rect.bottom; ++y) {
> > + for (int i = 0; i < 3; i++) { // ignore alpha
> > + int p = x * 4 + y * _stride + i;
> > + if (other._data[p] != _data[p]) {
> > + spice_printerr("equal fails at (+%d+%d) +%d+%d:%d
> > in %dx%d",
> > + rect.left, rect.top, x-rect.left,
> > y-rect.top, i,
> > + _width-rect.left,
> > _height-rect.top);
> > + if (getenv("DIFFBP"))
> > + SPICE_BREAKPOINT();
> > + return false;
> > + }
> > + }
> > + }
> > + return true;
> > + }
> > +
> > protected:
> > Format _format;
> > int _width;
> > diff --git a/client/utils.h b/client/utils.h
> > index 4657279..d8d6962 100644
> > --- a/client/utils.h
> > +++ b/client/utils.h
> > @@ -50,6 +50,9 @@ private:
> > throw Exception(exption_string, err); \
> > }
> >
> > +#define SPICE_BREAKPOINT() do{ \
> > + __asm__ __volatile__ ("int $03"); \
> > +}while(0)
> >
> > template <class T>
> > class AutoRef {
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list