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

[Klaus Espenlaub]

Hi Sergey,

On 06.11.2013 19:34, Sergey Staroletov wrote:
> Hello, have anyone looked to this patch?:) Or it was interesting to us
> only?

I did look at it, but the latest diff was too close to the 4.3.0 release 
to have a chance to be included. Is there a way to verify the problem on 
any other host platform than OSX? We can test on OSX, but I wonder if 
really Windows itself will never produce newer DIB variants...

In other words: I'd wave this through with far less hesitation if the 
fix would be only active where the problem actually exists, and not "by 
chance", just because it's unlikely or impossible that on other host 
OSes we'll get into the new code path, simply because the conditions 
magically don't trigger.

> Our client wants it to be opensource.

Oh, we love to get contributions, don't get me wrong. This is just the 
very non-obvious type, and therefore I'm quite cautious. It doesn't help 
anyone if the end result is more crashes or garbled images through the 
clipboard sharing. Limiting the risk is important, especially if you 
want to have this feature backported into 4.3 or 4.2...

Klaus

>
>
> On 30.09.2013 11:53, Sergey Staroletov wrote:
>> 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.