Problems with OutputDevice

Wed Dec 30 19:09:46 UTC 2020

Hi all, 

Firstly, let me preface this by saying that most of you are far more experienced than I am with LibreOffice and C++. But in reality, you all know that LO is a bit of a hairball. When I started working on LibreOffice (and making many mistakes) it literally took me years to understand how only one module worked. That module was the VCL. 

The VCL, like any ancient library, has gathered cruft for over a decade, maybe more. However, the key design decisions seem to me to be quite sound. They are: to be truly cross-platform, migrate the platform specific code into it’s own class and allow this class to be derived by each of the platforms which will then implement the key functionality. 

The two key classes in this regard are SalGraphics and OutputDevice. The problem is that these classes don’t respect their boundaries. 

Leaky abstraction

OutputDevice essential works as a facade over the SalGraphics class. SalGraphics works out the nitty-gritty of actually drawing to a particular platform, and OutputDevice, theoretically, just calls on the SalGraphics primitives to draw to the graphics device it is targeting.  Except when it doesn’t. As an example, I started a WIP topic on gerrit to see what would happen if I shifted functionality that is in OutputDevice that does drawing into SalGraphics. 

The code can be found here:

https://gerrit.libreoffice.org/q/topic:%22SalGraphicsFallback%22+(status:open%20OR%20status:merged) <https://gerrit.libreoffice.org/q/topic:%22SalGraphicsFallback%22+(status:open%20OR%20status:merged)>

The problem is that we have a leaky abstraction. Over the years, a bunch of new primitive drawing functions have been added to SalGraphics, things like DrawPolyLine(), DrawPolyLine(), DrawPolygon(), DrawPolyPolygon(), etc. One of the first things you notice about these newly added drawing functions is that they return a boolean. The reason is because not all backends have implemented these primitives. If the backend doesn’t implement the primitive (I’m look at the X11 backend here in particular) then it just returns a false. The code that calls on it is then expected to work out what to do. 

This causes us to do all sorts of backfiips to implement the functionality in OutputDevice itself. We now have some incredibly convoluted code, merely to handle cases when SalGraphics cannot do what it needs to do - draw primitives. It means that instead of the clean separation of drawing in SalGraphics and using OutputDevice as a facade to call upon primitive graphics code, it also takes over the actual drawing itself. 

I propose that it might be worthwhile moving the fallback code back into SalGraphics itself. Any new primitive functions need to have fallback code, or we mandate that any new backends must implement the primitive function. My POC can be found above on gerrit. 

OutputDevice does too much

As I have been reading and attempting to unravel the ball of string that is OutputDevice, I often find cases where it implements things it shouldn’t. Here’s an example - OutputDevice converts a bitmap into a metafile. It’s true! See the following gerrit requests I’ve put through:

https://gerrit.libreoffice.org/c/core/+/108041/ <https://gerrit.libreoffice.org/c/core/+/108041/> 
https://gerrit.libreoffice.org/c/core/+/108042 <https://gerrit.libreoffice.org/c/core/+/108042>

As you can see, my proposal it to move this functionality out of OutputDevice - where it is not needed - to a single function in it’s own namespace. 

Here’s another example: OutputDevice takes a Gradient and converts it to a grayscale. Why does OutputDevice do this? Shouldn’t Gradient do this? I think so - I have submitted a patch to do this very thing:

https://gerrit.libreoffice.org/c/core/+/108374 <https://gerrit.libreoffice.org/c/core/+/108374> 

Another example - OutputDevice currently gets an emphasis mark. The only thing that is even remotely related to this code is the Y DPI value. Again, it does seem to me that OutputDevice is yet again doing things that it shouldn’t be. (Lest anyone think it’s just a whinge - and yes it is a bit - I have submitted a patch for this also - https://gerrit.libreoffice.org/c/core/+/108457/ <https://gerrit.libreoffice.org/c/core/+/108457/>)

It doesn’t take long to go through the code to find examples of this. I challenge anyone to look at the gradient code in vcl/source/outdev/gradient.cxx and not see functionality that probably should be in the Gradient class. Need to remove transparencies from a metafile? OutputDevice has you covered! Need a font charmap? Get it from OutputDevice. 

OutputDevice cares about state too much

OutputDevice is tangled up in state flags. As an example, there is a flag called DrawModeFlags. These flags are set to do things like change drawing to greyscale, or to prevent filling… all sorts of things that you would think that should normally be passed to the drawing function. It confuses the code, and causes odd code pathways. I shall pick on the DrawGradient() code again, not because it is any worse than other code I’ve seen, but because it’s code I’ve been trying to whip into shape for some time now. 

OutputDevice::DrawGradient() will draw a grayscale gradient if the draw mode flag is set to DrawModeFlags <https://opengrok.libreoffice.org/s?defs=DrawModeFlags&project=core>::GrayGradient <https://opengrok.libreoffice.org/s?defs=GrayGradient&project=core>. Instead of the code being forced to tell the Gradient to convert itself to a grayscale, you have to set the drawing flag on OutputDevice to DrawModeFlags <https://opengrok.libreoffice.org/s?defs=DrawModeFlags&project=core>::GrayGradient <https://opengrok.libreoffice.org/s?defs=GrayGradient&project=core>, and then when you no longer wish to draw a grayscale gradient you have to turn that flag off. That’s a lot of state for someone to keep in mind. The thing is, some of the flags can cause side effects in other functions. Keeping track of what this does is a bit of a nightmare. 

Other state that you need to set is: text color, text fill color, text line color, overline color and text alignment. We do the same for lines. We also set the current font that we are trying to draw. 

OutputDevice is tightly wrapped up in GDIMetaFile

Every drawing function has a dual purpose in that it records itself to a metafile. This is insanity! I cannot understand why we are doing this - I’m sure there was a reason back in the day, but maintaining a metafile leads to ridiculous situations in the code where before you draw a polygon you must temporarily copy the pointer to the OutputDevice metafile, null the mpMetafile member variable, and then when you are done with the drawing you restore the old pointer.

There are other occasions where you need to populate the metafile, so you must disable output via EnableOutput(false). Go figure?

When drawing a line, you don’t pass it a fill or line color. Instead, you set the line and fill color. The function literally has to check if these colors are initialised before each drawing. This seems to be done because again, we are recording to a metafile that is as close to Microsoft's metafiles as possible. Heck, half the time we push these colors to a stack and then pop them off when we are done. 

(Just a few other things we also store in OutputDevice - emphasis ascent and descent (!))

OutputDevice relies of VirtualDevice for alpha blending

What can I say? It’s an extraordinary hack and has been that way for a long time. 

OutputDevice is tightly coupled to its inherited classes

See https://bugs.documentfoundation.org/show_bug.cgi?id=74702 <https://bugs.documentfoundation.org/show_bug.cgi?id=74702> 

We have OUTDEV_WINDOW, OUTDEV_PRINTER, OUTDEV_VIRDEV and now OUTDEV_PDF. We also have OutDevViewType which can be DontKnow (!), PrintPreview and Slideshow. It’s so tightly coupled that Printer, VirtualDevice, and not just Window but WorkWindow are friends of OutputDevice. This leads to all sorts of cases where the code needs to know what is actually calling on it - Printer, VirtualDevice or Window and so you see liberal sprinklings of if statements that check on this flag all through the code. 

Thankfully many years ago I saw that OutputDevice inherited from the Resource class for no good reason and was able to quickly remove this inheritance.  

Conclusion

These are my observations on the state of OutputDevice. No doubt some of you disagree that this state of affairs is so bad. There may even be disagreement that OutputDevice is doing anything it shouldn’t be. I don’t see any plans published though to do something about it. I have some ideas, one of which is to move the fallback drawing code into SalGraphics. That alone will cut back unnecessary code considerably. 

I would be curious what others think. 

Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libreoffice/attachments/20201231/c297d93e/attachment.htm>