Issue 120165

Summary: [From Symphony]Impress crashed when play screen show with sample file
Product: Impress Reporter: Yan Ji <yanji.yj>
Component: editingAssignee: AOO issues mailing list <issues>
Status: CLOSED FIXED QA Contact:
Severity: Critical    
Priority: P3 CC: Armin.Le.Grand, awf.aoo, liushenf, louqingle, yunquanp
Version: 3.4.0   
Target Milestone: 4.0.0   
Hardware: All   
OS: All   
Issue Type: DEFECT Latest Confirmation in: ---
Developer Difficulty: ---
Description Flags
Fix patch for Bug 120165
Fix patch for Bug 120165
yunquanp: review?
Fix by correcting Mask/Alpha at BitmapEx construction time none

Description Yan Ji 2012-07-03 08:01:28 UTC
Created attachment 78561 [details]

Build: AOO 3.4

Open attached sample file, then play screen show

Defect: Application crashed
Comment 1 pengyunquan 2012-07-27 03:43:48 UTC
Created attachment 78747 [details]
Fix patch for Bug 120165

The sample file contains .svm files which contains bitmapex whose mask is not the same size as its bitmap. And this cause access out of boundary in GraphicManager::ImplCreateScaled
Comment 2 pengyunquan 2012-07-27 03:46:29 UTC
Comment on attachment 78747 [details]
Fix patch for Bug 120165

update a patch with full source file path later
Comment 3 pengyunquan 2012-07-27 04:37:00 UTC
Created attachment 78748 [details]
Fix patch for Bug 120165

Update fix patch
Comment 4 Andre 2012-07-27 08:00:28 UTC
@pengyunquan: I have two questions regarding the fix:
1. Instead of introducing two new variables nDstWV and nDstHV, would it be possible to modify and reuse the existing ones nDstW and nDstH?

2. The use pMapLX and pMapLY in lines 1251 and 1252 indicates that their values are sorted.  Is that true?
Comment 5 Armin Le Grand 2012-07-27 09:40:19 UTC
ALG: I have even more questions to this crash and fix.
- Why only fix for Mask, BitmapEx can also have Alpha with the same problem?
- Why not fix in a way that alpha/mask gets scaled and not just truncated?
- The BitmapEx constructor already asserts unequal sizes, this imples that unequal sizes are not wanted from design. Why not fix there?

I will propose an other xix/patch.
Comment 6 Armin Le Grand 2012-07-27 09:51:59 UTC
Created attachment 78754 [details]
Fix by correcting Mask/Alpha at BitmapEx construction time

ALG: Patch added which corrects given Alpha/Mask at BitmapEx construction time, thus ensuring unequal pixel sizes will not happen. This makes all code based on BitmapEx pixel magic involving Alpha/Mask more simple and safe.
Comment 7 Andre 2012-07-27 12:09:46 UTC
@ALG: Is this an additional patch or does it replace the previous one?

@pengyunquan: Is the second patch OK for you?
Comment 8 Armin Le Grand 2012-07-27 12:42:36 UTC
ALG: It replaces the original one. It fixes the general problem root by adapting the mask/alpha bitmap at BitmapEx construction time if not the same pixel size (there was an assert there already indicating that this is not wanted).
Comment 9 Andre 2012-07-27 14:39:53 UTC
Then go ahead and check in your changes.
Comment 10 SVN Robot 2012-07-27 15:04:44 UTC
"alg" committed SVN revision 1366405 into trunk:
#120165# Adapt Mask/Alpha at BitmapEx construction when size differs from bas...
Comment 11 Armin Le Grand 2012-07-27 15:05:10 UTC
ALG: Okay, comitted as revision 1366405, done.
Comment 12 pengyunquan 2012-07-31 06:15:46 UTC
I have one question: If we scale the mask bitmap, is the mask bitmap still a valid mask? As we know even a pixel offset of mask bitmap will get quite different mask result.
Comment 13 pengyunquan 2012-07-31 06:23:42 UTC
If we scale the mask bitmap, we changed the saved BitmapEx object. I know it is an abnormal BitmapEx object, and we don't know how this abnormal object is generated. But I think we'd better keep the abnormal BitmapEx object to avoid risk of incorrect display result.
Comment 14 Armin Le Grand 2012-07-31 07:27:57 UTC
ALG->pengyunquan: When the mask has not the same pixel size, it can anyways not fit the content; the mask will anyways been scaled when painting with VCL (not truncated as in the patch), so scaling it at BitmapEx construction time makes no difference for visualization, except that the visualisation does the scaling at runtime.
Changing the saved BitmapEx: Yes, it will be saved changed, but I think this is acceptable for such bitmaps. As explained above, VCL will scale the Mask/Alpha at output to the same output pixel size anyways. All pixel formats I know which support Alpha/Mask have something like RGBA anyways in file formats, thus a unequal SizePixel is not even possible/allowed since one alpha value is bound to one pixel.
I also do not know how that BitmapEx was created, but as the existing asserts show it is not intended and leads to many exceptions in the whole office. In many places are asserts to warn in this case (as in the task case there were already warnings), but no handling implemented. I have not designed VCL, but this looks like purpose, too.
If there should really be a case where this is problematic we can react on it when it comes up.
Comment 15 louqle 2012-08-20 07:18:56 UTC
verified on XP SP3 and Ubuntu 12.04 against trunk build 1374181
Comment 16 louqle 2012-08-23 00:52:10 UTC
Verified on xp, Ubuntu, win7,mac,win7-64,Vista,suse,redhat, SLED 11 SP1 64 bit, all pass, close this bug
Comment 17 Shenfeng Liu 2012-11-07 08:36:00 UTC
set Target Milestone to AOO 3.5.0 for PM purpose.