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: [email protected]
--- VBoxClipboard_trunk.cpp 2013-09-30 17:27:28.348731129 +0700
+++ VBoxClipboard_v2.cpp 2013-09-30 17:27:29.060731131 +0700
@@ -129,6 +129,135 @@
pCtx->fCBChainPingInProcess = FALSE;
}
+static HANDLE vboxRgbDibToDibV1(HANDLE dibHandle)
+{
+ PBITMAPINFOHEADER dibHeaderData = NULL;
+ void *dibRawData = 0;
+
+ bool changeOrientation = false;
+ bool changeHeader = false;
+
+ HANDLE dibv1Handle = NULL;
+ void *dibv1Data = NULL;
+ PBITMAPINFOHEADER dibv1Header = NULL;
+
+ DWORD oldHeaderSize = 0;
+ LONG width;
+ LONG height;
+ SIZE_T rawDataLineSize = 0;
+ SIZE_T rawDataLinePadding = 0;
+
+ if (dibHandle == NULL)
+ {
+ return dibHandle;
+ }
+
+ dibHeaderData = static_cast<PBITMAPINFOHEADER>(GlobalLock(dibHandle));
+
+ if (dibHeaderData == NULL)
+ {
+ /* Can't lock dibHandle */
+ return dibHandle;
+ }
+
+ if (dibHeaderData->biCompression != BI_RGB)
+ {
+ /* Leave compressed images or image with pallete as is */
+ return dibHandle;
+ }
+
+ width = dibHeaderData->biWidth;
+ height = dibHeaderData->biHeight;
+
+ if (dibHeaderData->biBitCount < 8 ||
+ width <= 0 ||
+ dibHeaderData->biHeight == 0)
+ {
+ /* Leave 8bpp or empty image as it */
+ return dibHandle;
+ }
+
+ if (height < 0)
+ {
+ /* Upside down image */
+ changeOrientation = true;
+ height *= -1;
+ }
+
+ if (dibHeaderData->biSize != sizeof(BITMAPINFOHEADER))
+ {
+ /* Header version is not equal 1 */
+ changeHeader = true;
+ }
+
+ if (!changeOrientation && !changeHeader)
+ {
+ /* Nothing changed */
+ return dibHandle;
+ }
+
+ rawDataLineSize = (dibHeaderData->biBitCount/8)*(dibHeaderData->biWidth);
+
+ /* The size of each row is rounded up to a multiple of 4 bytes (a 32-bit DWORD) by padding. */
+ rawDataLinePadding = rawDataLineSize%4;
+ if (rawDataLinePadding > 0)
+ {
+ rawDataLineSize += 4 - rawDataLinePadding;
+ }
+
+ /* Create new handle */
+ dibv1Handle = GlobalAlloc(GMEM_DDESHARE | GMEM_MOVEABLE, sizeof(BITMAPINFOHEADER) + rawDataLineSize*height);
+
+ if (dibv1Handle == NULL)
+ {
+ /* Can't alloc handle */
+ return dibHandle;
+ }
+
+ dibv1Data = GlobalLock(dibv1Handle);
+
+ if (dibv1Data == NULL)
+ {
+ /* Can't lock handle */
+ GlobalFree(dibv1Handle);
+ return dibHandle;
+ }
+
+ dibv1Header = static_cast<PBITMAPINFOHEADER>(dibv1Data);
+
+ /* Copy header anyway */
+ memcpy((void*)dibv1Header, (void*)dibHeaderData, sizeof(BITMAPINFOHEADER));
+ dibv1Header->biSize = sizeof(BITMAPINFOHEADER);
+ dibv1Header->biHeight = height;
+
+ if (changeOrientation)
+ {
+ /* Copy data with correct orientation.
+ * Start from last row in source data and first row in distance.
+ */
+ char *psIter = (char*)dibHeaderData + dibHeaderData->biSize + rawDataLineSize*(height - 1) ;
+ char *pdIter = (char*)dibv1Data + sizeof(BITMAPINFOHEADER);
+ for (int i = 0; i < height; ++i)
+ {
+ memcpy(pdIter, psIter, rawDataLineSize);
+ psIter -= rawDataLineSize;
+ pdIter += rawDataLineSize;
+ }
+ }
+ else
+ {
+ /* Copy data */
+ memcpy((void*)((char*)dibv1Data + sizeof(BITMAPINFOHEADER)),
+ (void*)((char*)dibHeaderData + dibHeaderData->biSize),
+ rawDataLineSize*height);
+ }
+
+ GlobalUnlock(dibHandle);
+ GlobalUnlock(dibv1Handle);
+
+ return dibv1Handle;
+}
+
static LRESULT vboxClipboardProcessMsg(VBOXCLIPBOARDCONTEXT *pCtx, HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
{
LRESULT rc = 0;
@@ -349,15 +478,39 @@
if (hMem)
{
/* 'hMem' contains the host clipboard data.
- * size is 'cb' and format is 'format'. */
- HANDLE hClip = SetClipboardData(format, hMem);
+ * size is 'cb' and format is 'format'.
+ * 'dataMem' contains data converted to dibV1
+ */
+ HANDLE dataMem = hMem;
+ if (format == CF_DIB)
+ {
+ dataMem = vboxRgbDibToDibV1(hMem);
+ if (dataMem == NULL)
+ {
+ dataMem = hMem;
+ }
+ }
+ HANDLE hClip = SetClipboardData(format, dataMem);
Log(("VBoxTray: vboxClipboardProcessMsg: WM_RENDERFORMAT hClip = %p\n", hClip));
if (hClip)
{
- /* The hMem ownership has gone to the system. Finish the processing. */
+ /* The dataMem ownership has gone to the system. Finish the processing. */
+ if (hMem != dataMem)
+ {
+ /* Cleanup hMem if new Dib data created */
+ GlobalFree(hMem);
+ }
break;
}
+ else
+ {
+ /* Clean up dib dataMem if needed */
+ if (hMem != dataMem)
+ {
+ GlobalFree(dataMem);
+ }
+ }
/* Cleanup follows. */
}
@@ -725,4 +878,3 @@
memset (pCtx, 0, sizeof (*pCtx));
return;
}
-
_______________________________________________
vbox-dev mailing list
[email protected]
https://www.virtualbox.org/mailman/listinfo/vbox-dev
