[vbox-dev] [PATCH] VirtualBox Additions DIB Patch

[Sergey Staroletov]

Hi, we improved the patch a little bit, please see comments.


On 16.09.2013 14:12, Klaus Espenlaub wrote:
> Hi Sergey,



> On 12.09.2013 13:39, Sergey Staroletov wrote:
>> Hello, did anybody found our patch useful? Why not to include it to 
>> the
>> trunk?
>
> The patch would be trivial to integrate if its code wouldn't 
> contradict
> its comments and would be more complete/correct :)
>
> For example the comments hint that BI_BITFIELDS are not handled, but 
> the
> check in the code accepts both BI_RGB and BI_BITFIELDS and proceeds 
> with
> them. For BI_BITFIELDS it will not cause trouble if height is 
> positive
> (or if it is already DIBv1 - but still the code has to be more
> consistent. I suspect it shreds the data in the code path which 
> changes
> orientation or header size.
>

Now the code matches this comment, images with BI_BITFIELDS are passing 
and aren't processed with our function.
Also we are skipping compressed images, images with palette and images 
with null height or width.


> Also, it's unacceptable that the code malfunctions (for upside down
> images or if the code recreates the header which actually shouldn't 
> ever
> happen) if there is a color table (8bpp or less) or is BI_BITFIELDS. 
> It
> silently drops the color table, making the DIB violate the spec. The
> result will be unpredictable, ranging from image corruption to 
> crashes.
> Depending on what images the user is dealing with I have the 
> impression
> that there can be more crashes than before.

8bpp checking was in the old code. Now all images with palette are 
skipping, that's why copying is
processing right. If we required to flip image with DIB header version 
1, we don't change the header but just copy it, and put the flipped data 
behind it.


> Can you provide a patch with those issues addressed? Would speed up
> integration, as we'd first have to dig into the subtleties of the DIB
> format before we can efficiently bring your contribution to 
> production
> quality.
>
> Thanks for trying to improve VirtualBox, we appreciate any 
> contributions,
>
> Klaus
>
>>
>> Thanks, Sergey
>>
>> On 19.06.2013 10:49, Sergey Staroletov wrote:
>>> Hi François,
>>>
>>> On 19.06.2013 09:33, François Revol wrote:
>>>> Thanks for working on this, I knew it wasn't fully working when I
>>>> wrote
>>>> the initial code, but it worked for me, and I don't have a Mac
>>>> anymore
>>>> anyway.
>>>
>>>> Of course best would be to settle on a really standardized (PNG?)
>>>> format
>>>> for interchange instead of a "mostly documented" one, but well...
>>>>
>>>
>>> Yes may be it a better solution, but we worked on it for one client
>>> and we can release only this patch as he wants it to be opensource.
>>>
>>>>>
>>>>> We modified VBoxClipboard.cpp. We were used VirtualBox-4.2.12
>>>>> guests for
>>>>> tests.
>>>>
>>>> It's usually best to submit diffs in unified format (diff -u) so
>>>> that
>>>> they contain context lines around for patch to make sure it 
>>>> doesn't
>>>> break anything.
>>>>
>>>> Also, your patch has much higher chances of getting in if you diff
>>>> against the current svn trunk, in which case "svn diff" will 
>>>> produce
>>>> the
>>>> unified format automatically.
>>>
>>>
>>> no problem, here is diff -u for the current trunk.
>>>
>>>
>>>> François.

-- 
Sergey Staroletov
Senior UNIX C++ developer
Sibers (HireRussians/former Key-Soft Ltd.)
E-mail: sergey.staroletov at sibers.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: VBoxClipboard.diff
Type: text/x-diff
Size: 6293 bytes
Desc: not available
URL: <http://www.virtualbox.org/pipermail/vbox-dev/attachments/20130930/b3c996e5/attachment.bin>