[Libreoffice-bugs] [Bug 118029] crashtesting ooo24656-1.doc with --convert-to pdf

[bugzilla-daemon at bugs.documentfoundation.org]

https://bugs.documentfoundation.org/show_bug.cgi?id=118029

--- Comment #4 from Noel Grandin <noelgrandin at gmail.com> ---
IRC discussion:

<noelgrandin> caolan__, I am looking at that Bitmap crashtesting report
<loircbot> LibreOffice (online) fitojb * favicon.ico: Favicon refresh
<caolan__> noelgrandin, thanks, looks like it used to work cause the
BitmapReadAccess was taken in the ctor originally before the threading kicked
in
<noelgrandin> caolan__, ah, interesiting. Currently I am trying to make
BitmapInfoAccess hold the underlying SalBitmap rather than the Bitmap, do you
have an easier fix?
<noelgrandin> caolan__, part of the threading problem is also that MapMode is a
non-threadsafe COW thingy
<caolan__> noelgrandin, I'm not sure what the Bitmap vision is, is
BitmapReadAccess to go away ?
<noelgrandin> caolan__, and MapMode is part of Bitmap, hence my idea to bypass
Bitmap
<noelgrandin> caolan__, the vision (AFAIK) is to dump Bitmap, and have BitmapEx
hold SalBitmap
<noelgrandin> and have almost everything bitmap related (other than BitmapEx)
hidden inside vcl
<noelgrandin> mmeeks, quikee ^^^
<quikee> the vision is to merge Bitmap and BitmapEx into BitmapEx
<mmeeks> caolan__: the big plan is to get a single Alpha bitmap (and drawing
surface) - so we don't render everything twice in alpha mode at huge cost =)
<thorsten> noelgrandin: uh - Bitmap itself is COW, so why is mapmode making any
difference? ;)
<caolan__> well, I guess if there was a PixelGetter or whatever which was
created where BitmapReadAccess used to be created, and use used where the
GetPixel is called, then the problem wouldn't arise
<mmeeks> caolan__: step #1 - bring all the separate pixel-bashing madness into
VCL so we can surround & kill it ;-)
<noelgrandin> thorsten, because, in this case, we are accessing Bitmap via
multiple threads, and Bitmap holds MapMode, and MapMode is not threadsafe
<mmeeks> noelgrandin: really? =)
* mmeeks interested in that - but about to join a call ;-)
<mmeeks> noelgrandin: surely ~all bitmap use (except perhaps during parallel
image loading) is protected by the SolarMutex
<noelgrandin> mmeeks, apparently not in the PDF export code
<noelgrandin> or more accurately PDF convert-to code
<noelgrandin> but if that is the preferred solution I can do that instead
<thorsten> noelgrandin: use cow_wrapper<Foo, ThreadSafeRefCountingPolicy> then?
<noelgrandin> thorsten, that occurred to me, but wasn't sure it wouldn't impact
perofmance elswhere, not sure how widely used that thing is
<thorsten> noelgrandin: impossible to tell from the outset ;)
<thorsten> but yeah, rather widely used..
<noelgrandin> thorsten, and in this case, Bitmap holds SalBitmap via
std::shared_ptr and BitmapInfoAccess has no real need to hold a Bitmap, it can
get all the info it needs directly from the SalBitmap
<thorsten> not sure how often it gets copied/shared though
<quikee> it uses atomic - that should have a minimum inpact?
<noelgrandin> quikee, I could try that, but I figured we're going to bypass
Bitmap at some point anyhow, so I'll just get an early start :_
<quikee> noelgrandin: the problem will shift from Bitmap to BitmapEx only
<noelgrandin> quikee, in this case, I only have to fix thread-safe reading from
bitmap data
<quikee> but yeah... MapMode sux in general
<noelgrandin> not even sure what all MapMode is used for
<quikee> it is used to cause me headache
<noelgrandin> and given it's rather small size, why it needs to be COW.
<noelgrandin> :-)
<thorsten> noelgrandin: so for vcl stuff, someone at some time surely did some
assessment, perhaps even profiling,
<thorsten> noelgrandin: but that might well have been in 1994, so results
likely are entirely irrelevant today
<vmiklos> noelgrandin: MapMode is a way to describe how to map from logic units
to pixels; given doc model is in logic units (twips or mm100 usually) and the
screen is in pixels, it's used ~everywhere
<noelgrandin> thorsten, yeah, just commenting, have no intention of changing
that, (other than maybe trying the thread-safe COW thing if my current attempt
doesn't work out)
<thorsten> noelgrandin: well if you touch the area anyway, why not un-cow the
thing - if even strings were found to benefit from just plain copying ...
<quikee> vmiklos: sure but this mapping shouldn't be done ~everywhere :)
<noelgrandin> hah, and while doing this, found a bug originating from year
2000!
<vmiklos> quikee: agreed
<quikee> My vision for this is to do everything in logical units, converted to
the EMU, conversion logic to pixel would only be done in the OutpuDevice. The
exception is the UI only, where the conversion would be highly controlled.
<thorsten> quikee: mmh - be aware that Calc especially does a lot of
pixel-aligning still
<thorsten> quikee: also Writer is full of trickery, but there at least that's
prolly worth killing for good
<noelgrandin> hooo boy, looking deeper, I think I am going to need to just use
the SolarMutex hammer on this thing.
<noelgrandin> the various *SalBitmap::AcquireBuffer methods do not look
threadsafe at all, even for read-only usage
<SophiaS> Oh, seems related: https://gerrit.libreoffice.org/#/c/55365/
<quikee> thorsten: yes, I know - it is just a vision for now :)
<quikee> noelgrandin: there is nothing thread-safe there
<noelgrandin> quikee, then I'm afraid the SolarMutex it will need to be
<buovjaga> alg: is it temporary that SVGs open in Impress?
<alg> buovjaga: no, by purpose. Idea is to have roundtrip oneday when you save
impress as svg now
<alg> buovjaga: was even hard to do :-)
<quikee> noelgrandin: that could have performance implications
<noelgrandin> quikee, alternatives?
<quikee> noelgrandin: none
<buovjaga> alg: ok, you are relieved to hear I did not report a bug :P

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libreoffice-bugs/attachments/20180606/f37e1281/attachment-0001.html>