[PATCH v2 1/5] ole32: Add support for loading enhmetafiles from presentation and contents streams to data cache.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/ole32/datacache.c | 185 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 122 insertions(+), 63 deletions(-)

diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
index 325a98b33b..799e14a867 100644
--- a/dlls/ole32/datacache.c
+++ b/dlls/ole32/datacache.c
@@ -459,6 +459,45 @@ static void DataCache_FireOnViewChange(
   }
 }

+/* helper for copying STGMEDIUM of type bitmap, MF, EMF or HGLOBAL.
+* does no checking of whether src_stgm has a supported tymed, so this should be
+* done in the caller */
+static HRESULT copy_stg_medium(CLIPFORMAT cf, STGMEDIUM *dest_stgm,
+                               const STGMEDIUM *src_stgm)
+{
+    if (src_stgm->tymed == TYMED_MFPICT)
+    {
+        const METAFILEPICT *src_mfpict = GlobalLock(src_stgm->u.hMetaFilePict);
+        METAFILEPICT *dest_mfpict;
+
+        if (!src_mfpict)
+            return DV_E_STGMEDIUM;
+        dest_stgm->u.hMetaFilePict = GlobalAlloc(GMEM_MOVEABLE, sizeof(METAFILEPICT));
+        dest_mfpict = GlobalLock(dest_stgm->u.hMetaFilePict);
+        if (!dest_mfpict)
+        {
+            GlobalUnlock(src_stgm->u.hMetaFilePict);
+            return E_OUTOFMEMORY;
+        }
+        *dest_mfpict = *src_mfpict;
+        dest_mfpict->hMF = CopyMetaFileW(src_mfpict->hMF, NULL);
+        GlobalUnlock(src_stgm->u.hMetaFilePict);
+        GlobalUnlock(dest_stgm->u.hMetaFilePict);
+    }
+    else if (src_stgm->tymed != TYMED_NULL)
+    {
+        dest_stgm->u.hGlobal = OleDuplicateData(src_stgm->u.hGlobal, cf,
+                                                GMEM_MOVEABLE);
+        if (!dest_stgm->u.hGlobal)
+            return E_OUTOFMEMORY;
+    }
+    dest_stgm->tymed = src_stgm->tymed;
+    dest_stgm->pUnkForRelease = src_stgm->pUnkForRelease;
+    if (dest_stgm->pUnkForRelease)
+        IUnknown_AddRef(dest_stgm->pUnkForRelease);
+    return S_OK;
+}
+
 static HRESULT read_clipformat(IStream *stream, CLIPFORMAT *clipformat)
 {
     DWORD length;
@@ -547,6 +586,30 @@ static HRESULT open_pres_stream( IStorage *stg, int stream_number, IStream **stm
     return IStorage_OpenStream( stg, name, NULL, STGM_READ | STGM_SHARE_EXCLUSIVE, 0, stm );
 }

+static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
+{
+    METAFILEPICT *pict;
+    HRESULT hr = E_FAIL;
+    UINT size;
+    void *bits;
+
+    if (!(pict = GlobalLock( data ))) return hr;
+
+    size = GetMetaFileBitsEx( pict->hMF, 0, NULL );
+    if ((bits = HeapAlloc( GetProcessHeap(), 0, size )))
+    {
+        GetMetaFileBitsEx( pict->hMF, size, bits );
+        med->u.hEnhMetaFile = SetWinMetaFileBits( size, bits, NULL, pict );
+        HeapFree( GetProcessHeap(), 0, bits );
+        med->tymed = TYMED_ENHMF;
+        med->pUnkForRelease = NULL;
+        hr = S_OK;
+    }
+
+    GlobalUnlock( data );
+    return hr;
+}
+
 static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
 {
     HRESULT hr;
@@ -701,7 +764,62 @@ fail:
     GlobalUnlock( hglobal );
     GlobalFree( hglobal );
     return E_FAIL;
+}

+static HRESULT load_emf( DataCacheEntry *cache_entry, IStream *stm )
+{
+    HRESULT hr;
+
+    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
+    {
+        STGMEDIUM stgmed;
+
+        hr = load_mf_pict( cache_entry, stm );
+        if (SUCCEEDED( hr ))
+            hr = synthesize_emf( cache_entry->stgmedium.u.hMetaFilePict, &stgmed );
+        if (SUCCEEDED( hr ))
+        {
+            ReleaseStgMedium( &cache_entry->stgmedium );
+            hr = copy_stg_medium( CF_ENHMETAFILE, &cache_entry->stgmedium, &stgmed );
+            ReleaseStgMedium( &stgmed );
+        }
+    }
+    else
+    {
+        STATSTG stat;
+        void *data;
+        DWORD size_bits;
+        ULONG read;
+
+        hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
+
+        if (SUCCEEDED( hr ))
+        {
+            data = HeapAlloc( GetProcessHeap(), 0, stat.cbSize.u.LowPart );
+            if (!data) return E_OUTOFMEMORY;
+
+            hr = IStream_Read( stm, data, stat.cbSize.u.LowPart, &read );
+            if (hr != S_OK || read != stat.cbSize.u.LowPart)
+            {
+                HeapFree( GetProcessHeap(), 0, data );
+                return E_FAIL;
+            }
+
+            size_bits = read - sizeof(DWORD) - sizeof(ENHMETAHEADER);
+            if (size_bits <= 0)
+            {
+                HeapFree( GetProcessHeap(), 0, data );
+                return E_FAIL;
+            }
+            cache_entry->stgmedium.u.hEnhMetaFile = SetEnhMetaFileBits( size_bits, (BYTE *)data + (read - size_bits) );
+            cache_entry->stgmedium.tymed = TYMED_ENHMF;
+            cache_entry->stgmedium.pUnkForRelease = NULL;
+
+            HeapFree( GetProcessHeap(), 0, data );
+        }
+    }
+
+    return hr;
 }

/************************************************************************
@@ -736,6 +854,10 @@ static HRESULT DataCacheEntry_LoadData(DataCacheEntry *cache_entry, IStorage *st
         hr = load_dib( cache_entry, stm );
         break;

+    case CF_ENHMETAFILE:
+        hr = load_emf( cache_entry, stm );
+        break;
+
     default:
         FIXME( "Unimplemented clip format %x\n", cache_entry->fmtetc.cfFormat );
         hr = E_NOTIMPL;
@@ -1030,45 +1152,6 @@ static HRESULT DataCacheEntry_Save(DataCacheEntry *cache_entry, IStorage *storag
     return hr;
 }

-/* helper for copying STGMEDIUM of type bitmap, MF, EMF or HGLOBAL.
-* does no checking of whether src_stgm has a supported tymed, so this should be
-* done in the caller */
-static HRESULT copy_stg_medium(CLIPFORMAT cf, STGMEDIUM *dest_stgm,
-                               const STGMEDIUM *src_stgm)
-{
-    if (src_stgm->tymed == TYMED_MFPICT)
-    {
-        const METAFILEPICT *src_mfpict = GlobalLock(src_stgm->u.hMetaFilePict);
-        METAFILEPICT *dest_mfpict;
-
-        if (!src_mfpict)
-            return DV_E_STGMEDIUM;
-        dest_stgm->u.hMetaFilePict = GlobalAlloc(GMEM_MOVEABLE, sizeof(METAFILEPICT));
-        dest_mfpict = GlobalLock(dest_stgm->u.hMetaFilePict);
-        if (!dest_mfpict)
-        {
-            GlobalUnlock(src_stgm->u.hMetaFilePict);
-            return E_OUTOFMEMORY;
-        }
-        *dest_mfpict = *src_mfpict;
-        dest_mfpict->hMF = CopyMetaFileW(src_mfpict->hMF, NULL);
-        GlobalUnlock(src_stgm->u.hMetaFilePict);
-        GlobalUnlock(dest_stgm->u.hMetaFilePict);
-    }
-    else if (src_stgm->tymed != TYMED_NULL)
-    {
-        dest_stgm->u.hGlobal = OleDuplicateData(src_stgm->u.hGlobal, cf,
-                                                GMEM_MOVEABLE);
-        if (!dest_stgm->u.hGlobal)
-            return E_OUTOFMEMORY;
-    }
-    dest_stgm->tymed = src_stgm->tymed;
-    dest_stgm->pUnkForRelease = src_stgm->pUnkForRelease;
-    if (dest_stgm->pUnkForRelease)
-        IUnknown_AddRef(dest_stgm->pUnkForRelease);
-    return S_OK;
-}
-
 static HRESULT synthesize_dib( HBITMAP bm, STGMEDIUM *med )
 {
     HDC hdc = GetDC( 0 );
@@ -1118,30 +1201,6 @@ static HRESULT synthesize_bitmap( HGLOBAL dib, STGMEDIUM *med )
     return hr;
 }

-static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
-{
-    METAFILEPICT *pict;
-    HRESULT hr = E_FAIL;
-    UINT size;
-    void *bits;
-
-    if (!(pict = GlobalLock( data ))) return hr;
-
-    size = GetMetaFileBitsEx( pict->hMF, 0, NULL );
-    if ((bits = HeapAlloc( GetProcessHeap(), 0, size )))
-    {
-        GetMetaFileBitsEx( pict->hMF, size, bits );
-        med->u.hEnhMetaFile = SetWinMetaFileBits( size, bits, NULL, pict );
-        HeapFree( GetProcessHeap(), 0, bits );
-        med->tymed = TYMED_ENHMF;
-        med->pUnkForRelease = NULL;
-        hr = S_OK;
-    }
-
-    GlobalUnlock( data );
-    return hr;
-}
-
 static HRESULT DataCacheEntry_SetData(DataCacheEntry *cache_entry,
                                       const FORMATETC *formatetc,
                                       STGMEDIUM *stgmedium,

[PATCH v2 2/5] ole32: Add support for loading dibs from presentation streams to data cache.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/ole32/datacache.c | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
index 799e14a867..e6cda331dd 100644
--- a/dlls/ole32/datacache.c
+++ b/dlls/ole32/datacache.c
@@ -610,6 +610,20 @@ static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
     return hr;
 }

+static HRESULT read_pres_header( IStream *stm, DWORD *clip_format,
+                                 PresentationDataHeader *pres_hdr )
+{
+    ULONG read;
+    HRESULT hr;
+
+    hr = IStream_Read( stm, clip_format, sizeof(DWORD) * 2, &read );
+    if (hr != S_OK || read != sizeof(DWORD) * 2) return E_FAIL;
+    hr = IStream_Read( stm, pres_hdr, sizeof(PresentationDataHeader), &read );
+    if (hr != S_OK || read != sizeof(PresentationDataHeader)) return E_FAIL;
+
+    return S_OK;
+}
+
 static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
 {
     HRESULT hr;
@@ -689,32 +703,42 @@ static HRESULT load_dib( DataCacheEntry *cache_entry, IStream *stm )
     void *dib;
     HGLOBAL hglobal;
     ULONG read, info_size, bi_size;
-    BITMAPFILEHEADER file;
     BITMAPINFOHEADER *info;
-
-    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
-    {
-        FIXME( "Unimplemented for presentation stream\n" );
-        return E_FAIL;
-    }
+    /* only standard clipformats */
+    struct {
+        DWORD cf[2];
+        PresentationDataHeader pres;
+    } pres_hdr;
+    BITMAPFILEHEADER file;

hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
     if (FAILED( hr )) return hr;

-    if (stat.cbSize.QuadPart < sizeof(file) + sizeof(DWORD)) return E_FAIL;
-    hr = IStream_Read( stm, &file, sizeof(file), &read );
-    if (hr != S_OK || read != sizeof(file)) return E_FAIL;
-    stat.cbSize.QuadPart -= sizeof(file);
+    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
+    {
+        if (stat.cbSize.QuadPart < sizeof(pres_hdr)) return E_FAIL;
+        hr = read_pres_header( stm, &pres_hdr, &pres_hdr.pres );
+        if (FAILED( hr )) return hr;
+        stat.cbSize.QuadPart -= sizeof(pres_hdr);
+    }
+    else
+    {
+        if (stat.cbSize.QuadPart < sizeof(BITMAPFILEHEADER)) return E_FAIL;
+        hr = IStream_Read( stm, &file, sizeof(BITMAPFILEHEADER), &read );
+        stat.cbSize.QuadPart -= sizeof(BITMAPFILEHEADER);
+    }

hglobal = GlobalAlloc( GMEM_MOVEABLE, stat.cbSize.u.LowPart );
     if (!hglobal) return E_OUTOFMEMORY;
     dib = GlobalLock( hglobal );

+    /* read first DWORD of BITMAPINFOHEADER */
     hr = IStream_Read( stm, dib, sizeof(DWORD), &read );
     if (hr != S_OK || read != sizeof(DWORD)) goto fail;
     bi_size = *(DWORD *)dib;
     if (stat.cbSize.QuadPart < bi_size) goto fail;

+    /* read rest of BITMAPINFOHEADER */
     hr = IStream_Read( stm, (char *)dib + sizeof(DWORD), bi_size - sizeof(DWORD), &read );
     if (hr != S_OK || read != bi_size - sizeof(DWORD)) goto fail;

@@ -727,7 +751,8 @@ static HRESULT load_dib( DataCacheEntry *cache_entry, IStream *stm )
     }
     stat.cbSize.QuadPart -= info_size;

-    if (file.bfOffBits)
+    /* set Stream pointer to beginning of bitmap bits */
+    if (cache_entry->load_stream_num == STREAM_NUMBER_CONTENTS && file.bfOffBits)
     {
         LARGE_INTEGER skip;

[PATCH v2 2/5] ole32: Add support for loading dibs from presentation streams to data cache.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:34PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/datacache.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
> index 799e14a867..e6cda331dd 100644
> --- a/dlls/ole32/datacache.c
> +++ b/dlls/ole32/datacache.c
> @@ -610,6 +610,20 @@ static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
>      return hr;
>  }
>
> +static HRESULT read_pres_header( IStream *stm, DWORD *clip_format,
> -
>                             PresentationDataHeader *pres_hdr )
>
> +{
> - ULONG read;
> - HRESULT hr;
> -
> - hr = IStream_Read( stm, clip_format, sizeof(DWORD) * 2, &read );
> - if (hr != S_OK || read != sizeof(DWORD) * 2) return E_FAIL;

Note, we already have read_clipformat() which correctly handles
the variable length stuff.  This should either call that or we
could have this function's callers do it themselves.  The latter
may be simpler, in which case this function ceases to be useful,
as you may as well do the IStream_Read in this function's callers.
> - hr = IStream_Read( stm, pres_hdr, sizeof(PresentationDataHeader), &read );
> - if (hr != S_OK || read != sizeof(PresentationDataHeader)) return E_FAIL;
> -
> - return S_OK;
>
> +}
> -
>
> static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
>  {
>      HRESULT hr;
> @@ -689,32 +703,42 @@ static HRESULT load_dib( DataCacheEntry *cache_entry, IStream *stm )
>      void *dib;
>      HGLOBAL hglobal;
>      ULONG read, info_size, bi_size;
> - BITMAPFILEHEADER file;
> BITMAPINFOHEADER *info;
> -
> - if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> - {
> -
>    FIXME( "Unimplemented for presentation stream\n" );
> -
>    return E_FAIL;
> - }
> - /* only standard clipformats */
> - struct {
> -
>    DWORD cf[2];
> -
>    PresentationDataHeader pres;
> - } pres_hdr;

This structure isn't very useful.  All you need a CLIPFORMAT and a
PresentationDataHeader as two separate variables.
> -
>
> BITMAPFILEHEADER file;
>
> hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
> if (FAILED( hr )) return hr;
> - if (stat.cbSize.QuadPart < sizeof(file) + sizeof(DWORD)) return E_FAIL;
> - hr = IStream_Read( stm, &file, sizeof(file), &read );
> - if (hr != S_OK || read != sizeof(file)) return E_FAIL;
> - stat.cbSize.QuadPart -= sizeof(file);
> -
>
> if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> -
>
> {
> -
>    if (stat.cbSize.QuadPart < sizeof(pres_hdr)) return E_FAIL;
> -
>    hr = read_pres_header( stm, &pres_hdr, &pres_hdr.pres );
> -
>    if (FAILED( hr )) return hr;
> -
>    stat.cbSize.QuadPart -= sizeof(pres_hdr);
> -
>
> }
> -
>
> else
> -
>
> {
> -
>    if (stat.cbSize.QuadPart < sizeof(BITMAPFILEHEADER)) return E_FAIL;
> -
>    hr = IStream_Read( stm, &file, sizeof(BITMAPFILEHEADER), &read );
> -
>    stat.cbSize.QuadPart -= sizeof(BITMAPFILEHEADER);
> -
>
> }
>
> hglobal = GlobalAlloc( GMEM_MOVEABLE, stat.cbSize.u.LowPart );
> if (!hglobal) return E_OUTOFMEMORY;
> dib = GlobalLock( hglobal );
> -
>
> /* read first DWORD of BITMAPINFOHEADER */
> hr = IStream_Read( stm, dib, sizeof(DWORD), &read );
> if (hr != S_OK || read != sizeof(DWORD)) goto fail;
> bi_size = *(DWORD *)dib;
> if (stat.cbSize.QuadPart < bi_size) goto fail;
> -
>
> /* read rest of BITMAPINFOHEADER */
> hr = IStream_Read( stm, (char *)dib + sizeof(DWORD), bi_size - sizeof(DWORD), &read );
> if (hr != S_OK || read != bi_size - sizeof(DWORD)) goto fail;
>
> @@ -727,7 +751,8 @@ static HRESULT load_dib( DataCacheEntry *cache_entry, IStream *stm )
>      }
>      stat.cbSize.QuadPart -= info_size;
> - if (file.bfOffBits)
> - /* set Stream pointer to beginning of bitmap bits */
> - if (cache_entry->load_stream_num == STREAM_NUMBER_CONTENTS && file.bfOffBits)
> {
>     LARGE_INTEGER skip;
>
> -- 
> 2.14.1

[PATCH v2 3/5] ole32: Add support for loading wmf from contents streams to data cache.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/ole32/datacache.c | 71 +++++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
index e6cda331dd..0f23faf08c 100644
--- a/dlls/ole32/datacache.c
+++ b/dlls/ole32/datacache.c
@@ -609,6 +609,17 @@ static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
     GlobalUnlock( data );
     return hr;
 }
+#include <pshpack2.h>
+struct meta_placeable
+{
+    DWORD key;
+    WORD hwmf;
+    WORD bounding_box[4];
+    WORD inch;
+    DWORD reserved;
+    WORD checksum;
+};
+#include <poppack.h>

static HRESULT read_pres_header( IStream *stm, DWORD *clip_format,
                                  PresentationDataHeader *pres_hdr )
@@ -629,28 +640,34 @@ static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
     HRESULT hr;
     STATSTG stat;
     ULARGE_INTEGER current_pos;
-    void *bits;
+    void *bits, *hdr;
     METAFILEPICT *mfpict;
     HGLOBAL hmfpict;
-    PresentationDataHeader header;
-    CLIPFORMAT clipformat;
     static const LARGE_INTEGER offset_zero;
-    ULONG read;
-
-    if (cache_entry->load_stream_num == STREAM_NUMBER_CONTENTS)
-    {
-        FIXME( "Unimplemented for CONTENTS stream\n" );
-        return E_FAIL;
-    }
+    ULONG read, hdr_size;
+    /* only standard clipformats */
+    struct {
+        DWORD cf[2];
+        PresentationDataHeader pres;
+    } pres_hdr;
+    struct meta_placeable meta_place_rec;

hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
     if (FAILED( hr )) return hr;

-    hr = read_clipformat( stm, &clipformat );
-    if (FAILED( hr )) return hr;
+    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
+    {
+        hdr_size = sizeof(pres_hdr);
+        hdr = &pres_hdr;
+    }
+    else
+    {
+        hdr_size = sizeof(struct meta_placeable);
+        hdr = &meta_place_rec;
+    }

-    hr = IStream_Read( stm, &header, sizeof(header), &read );
-    if (hr != S_OK || read != sizeof(header)) return E_FAIL;
+    hr = IStream_Read( stm, hdr, hdr_size, &read );
+    if (hr != S_OK || read != hdr_size) return E_FAIL;

hr = IStream_Seek( stm, offset_zero, STREAM_SEEK_CUR, &current_pos );
     if (FAILED( hr )) return hr;
@@ -673,10 +690,18 @@ static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )

if (SUCCEEDED( hr ))
     {
-        /* FIXME: get this from the stream */
         mfpict->mm = MM_ANISOTROPIC;
-        mfpict->xExt = header.dwObjectExtentX;
-        mfpict->yExt = header.dwObjectExtentY;
+        /* FIXME: get this from the stream */
+        if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
+        {
+            mfpict->xExt = pres_hdr.pres.dwObjectExtentX;
+            mfpict->yExt = pres_hdr.pres.dwObjectExtentY;
+        }
+        else
+        {
+            mfpict->xExt = (meta_place_rec.bounding_box[2] * 2540) / meta_place_rec.inch;
+            mfpict->yExt = (meta_place_rec.bounding_box[3] * 2540) / meta_place_rec.inch;
+        }
         mfpict->hMF = SetMetaFileBitsEx( stat.cbSize.u.LowPart, bits );
         if (!mfpict->hMF)
             hr = E_FAIL;
@@ -964,18 +989,6 @@ end:
     return hr;
 }

-#include <pshpack2.h>
-struct meta_placeable
-{
-    DWORD key;
-    WORD hwmf;
-    WORD bounding_box[4];
-    WORD inch;
-    DWORD reserved;
-    WORD checksum;
-};
-#include <poppack.h>
-
 static HRESULT save_mfpict(DataCacheEntry *entry, BOOL contents, IStream *stream)
 {
     HRESULT hr = S_OK;

[PATCH v2 3/5] ole32: Add support for loading wmf from contents streams to data cache.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:35PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/datacache.c | 71 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
> index e6cda331dd..0f23faf08c 100644
> --- a/dlls/ole32/datacache.c
> +++ b/dlls/ole32/datacache.c
> @@ -609,6 +609,17 @@ static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
>      GlobalUnlock( data );
>      return hr;
>  }
> +#include <pshpack2.h>
> +struct meta_placeable
> +{
> - DWORD key;
> - WORD hwmf;
> - WORD bounding_box[4];
> - WORD inch;
> - DWORD reserved;
> - WORD checksum;
>
> +};
> +#include <poppack.h>
>
> static HRESULT read_pres_header( IStream *stm, DWORD *clip_format,
>                                   PresentationDataHeader *pres_hdr )
> @@ -629,28 +640,34 @@ static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
>      HRESULT hr;
>      STATSTG stat;
>      ULARGE_INTEGER current_pos;
> - void *bits;
> - void *bits, *hdr;
> METAFILEPICT *mfpict;
> HGLOBAL hmfpict;
> - PresentationDataHeader header;
> - CLIPFORMAT clipformat;
> static const LARGE_INTEGER offset_zero;
> - ULONG read;
> -
> - if (cache_entry->load_stream_num == STREAM_NUMBER_CONTENTS)
> - {
> -
>    FIXME( "Unimplemented for CONTENTS stream\n" );
> -
>    return E_FAIL;
> - }
> -
>
> ULONG read, hdr_size;
> -
>
> /* only standard clipformats */
> -
>
> struct {
> -
>    DWORD cf[2];
> -
>    PresentationDataHeader pres;
> -
>
> } pres_hdr;
> -
>
> struct meta_placeable meta_place_rec;
>
> hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
> if (FAILED( hr )) return hr;
> - hr = read_clipformat( stm, &clipformat );
> - if (FAILED( hr )) return hr;
> - if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> - {
> -
>    hdr_size = sizeof(pres_hdr);
> -
>    hdr = &pres_hdr;
> - }
> - else
> - {
> -
>    hdr_size = sizeof(struct meta_placeable);
> -
>    hdr = &meta_place_rec;
> - }

I don't think setting up hdr and hdr_size in order to have a common
IStream_Read makes much sense.  Keep the original behaviour for the
presentation streams in one if block and implement the reading of
meta_placable in the else block.
> - hr = IStream_Read( stm, &header, sizeof(header), &read );
> - if (hr != S_OK || read != sizeof(header)) return E_FAIL;
> -
>
> hr = IStream_Read( stm, hdr, hdr_size, &read );
> -
>
> if (hr != S_OK || read != hdr_size) return E_FAIL;
>
> hr = IStream_Seek( stm, offset_zero, STREAM_SEEK_CUR, &current_pos );
> if (FAILED( hr )) return hr;
>
> @@ -673,10 +690,18 @@ static HRESULT load_mf_pict( DataCacheEntry *cache_entry, IStream *stm )
>  if (SUCCEEDED( hr ))
>  {
> -
>    /* FIXME: get this from the stream */
>    mfpict->mm = MM_ANISOTROPIC;
> -
>    mfpict->xExt = header.dwObjectExtentX;
> -
>    mfpict->yExt = header.dwObjectExtentY;
> -
>    /* FIXME: get this from the stream */
> -
>    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> -
>    {
> -
>        mfpict->xExt = pres_hdr.pres.dwObjectExtentX;
> -
>        mfpict->yExt = pres_hdr.pres.dwObjectExtentY;
> -
>    }
> -
>    else
> -
>    {
> -
>        mfpict->xExt = (meta_place_rec.bounding_box[2] * 2540) / meta_place_rec.inch;

This should probably be (bounding_box[2] - bounding_box[0]) * ...
unless you have evidence otherwise.

You may want to shorten "meta_place_rec" to something like "mf_place"...
> -
>        mfpict->yExt = (meta_place_rec.bounding_box[3] * 2540) / meta_place_rec.inch;
> -
>    }
>    mfpict->hMF = SetMetaFileBitsEx( stat.cbSize.u.LowPart, bits );
>    if (!mfpict->hMF)
>        hr = E_FAIL;
>
> @@ -964,18 +989,6 @@ end:
>      return hr;
>  }
>
> -#include <pshpack2.h>
> -struct meta_placeable
> -{
> - DWORD key;
> - WORD hwmf;
> - WORD bounding_box[4];
> - WORD inch;
> - DWORD reserved;
> - WORD checksum;
>
> -};
> -#include <poppack.h>
> -
>
> static HRESULT save_mfpict(DataCacheEntry *entry, BOOL contents, IStream *stream)
>  {
>      HRESULT hr = S_OK;
> -- 
> 2.14.1

[PATCH v2 4/5] ole32: Reset CLSID of datacache so that automatic entry is re-created on load.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/ole32/datacache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
index 0f23faf08c..452e171e4c 100644
--- a/dlls/ole32/datacache.c
+++ b/dlls/ole32/datacache.c
@@ -1862,6 +1862,7 @@ static HRESULT WINAPI DataCache_Load( IPersistStorage *iface, IStorage *stg )
         DataCacheEntry_Destroy( This, entry );

ReadClassStg( stg, &clsid );
+    This->clsid = CLSID_NULL;
     hr = create_automatic_entry( This, &clsid );
     if (FAILED( hr )) return hr;

[PATCH v2 4/5] ole32: Reset CLSID of datacache so that automatic entry is re-created on load.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:36PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/datacache.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
> index 0f23faf08c..452e171e4c 100644
> --- a/dlls/ole32/datacache.c
> +++ b/dlls/ole32/datacache.c
> @@ -1862,6 +1862,7 @@ static HRESULT WINAPI DataCache_Load( IPersistStorage *iface, IStorage *stg )
>          DataCacheEntry_Destroy( This, entry );
>  ReadClassStg( stg, &clsid );
> - This->clsid = CLSID_NULL;

This is probably hiding another bug.  In the new tests you add in
[5/5] the cache entries should have been created when the cache is
created.  Could you investigate what's going on?

Huw.

[PATCH v2 4/5] ole32: Reset CLSID of datacache so that automatic entry is re-created on load.

[Sergio Gómez Del Real]

On 03/04/18 05:12, Huw Davies wrote:
> On Fri, Mar 30, 2018 at 12:11:36PM -0500, Sergio Gómez Del Real wrote:
> > Signed-off-by: Sergio Gómez Del Real
> >
> > dlls/ole32/datacache.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
> > index 0f23faf08c..452e171e4c 100644
> > --- a/dlls/ole32/datacache.c
> > +++ b/dlls/ole32/datacache.c
> > @@ -1862,6 +1862,7 @@ static HRESULT WINAPI DataCache_Load( IPersistStorage *iface, IStorage *stg )
> >           DataCacheEntry_Destroy( This, entry );
> >   ReadClassStg( stg, &clsid );
> > - This->clsid = CLSID_NULL;
>
> This is probably hiding another bug.  In the new tests you add in
> [5/5] the cache entries should have been created when the cache is
> created.  Could you investigate what's going on?
>
> Huw.

In create_automatic_entry() we have:/
/

/if (IsEqualCLSID( &cache->clsid, clsid )) return S_OK;/

which I guess is there to prevent creating 2 static entries of the same 
CF. When data cache is created with a static picture, its clsid is set 
with one of CLSID_Picture_*, but only after calling 
create_automatic_entry(). Then, in _Load, the data cache is flushed (its 
entries destroyed), but its clsid isn't cleared; it maintains one of 
CLSID_Picture_* values. The patch does this clearance, which I think 
makes sense (since we are loading new entries and clsid from a storage), 
and so when create_automatic_entry() is called right after destroying 
the entries and reading the storage's clsid, it will succeed.

I would think that it is correct to 'reset' the clsid in _Load, as done 
in this patch, right after destroying the entries and before reading in 
the new entries and clsid from the storage; and also that 
create_automatic_entry() is correct to check the CLSIDs so as to avoid 
creating a new entry, which wouldn't be the desired behavior. What do 
you think?

- Sergio

[PATCH v2 4/5] ole32: Reset CLSID of datacache so that automatic entry is re-created on load.

[Huw Davies]

On Tue, Apr 03, 2018 at 06:13:09PM -0500, Sergio Gómez Del Real wrote:
> On 03/04/18 05:12, Huw Davies wrote:
>      On Fri, Mar 30, 2018 at 12:11:36PM -0500, Sergio Gómez Del Real
>      wrote:
>           Signed-off-by: Sergio Gómez Del Real
>           
>           ---
>            dlls/ole32/datacache.c | 1 +
>            1 file changed, 1 insertion(+)
>       diff --git a/dlls/ole32/datacache.c b/dlls/ole32/
>       datacache.c
>       index 0f23faf08c..452e171e4c 100644
>       --- a/dlls/ole32/datacache.c
>       +++ b/dlls/ole32/datacache.c
>       @@ -1862,6 +1862,7 @@ static HRESULT WINAPI DataCache_Load
>       ( IPersistStorage *iface, IStorage *stg )
>                DataCacheEntry_Destroy( This, entry );
>
>            ReadClassStg( stg, &clsid );
>       +    This->clsid = CLSID_NULL;
>  This is probably hiding another bug.  In the new tests you add in
>  [5/5] the cache entries should have been created when the cache is
>  created.  Could you investigate what's going on?
>
>  Huw.
>
> In create_automatic_entry() we have:
>      if (IsEqualCLSID( &cache->clsid, clsid )) return S_OK;
> which I guess is there to prevent creating 2 static entries of the same CF.
> When data cache is created with a static picture, its clsid is set with one of
> CLSID_Picture_*, but only after calling create_automatic_entry(). Then, in
> _Load, the data cache is flushed (its entries destroyed), but its clsid isn't
> cleared; it maintains one of CLSID_Picture_* values. The patch does this
> clearance, which I think makes sense (since we are loading new entries and
> clsid from a storage), and so when create_automatic_entry() is called right
> after destroying the entries and reading the storage's clsid, it will succeed.
>
> I would think that it is correct to 'reset' the clsid in _Load, as done in this
> patch, right after destroying the entries and before reading in the new entries
> and clsid from the storage; and also that create_automatic_entry() is correct
> to check the CLSIDs so as to avoid creating a new entry, which wouldn't be the
> desired behavior. What do you think?

Ok, I see, thanks.  Could you move it to above the blank line above
ReadClassStg(), so it sits in the 'destruction of cache entries block'?

Thanks,
Huw.

[PATCH v2 5/5] ole32/tests: Add tests for loading from presentation streams to data cache.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/ole32/tests/ole2.c | 165 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 145 insertions(+), 20 deletions(-)

diff --git a/dlls/ole32/tests/ole2.c b/dlls/ole32/tests/ole2.c
index 3bd4acce92..da9c5ff070 100644
--- a/dlls/ole32/tests/ole2.c
+++ b/dlls/ole32/tests/ole2.c
@@ -4078,6 +4078,74 @@ static void check_storage_contents(IStorage *stg, const struct storage_def *stg_
     }
 }

+static HRESULT check_medium_contents(const STGMEDIUM *stgmed_def, STGMEDIUM *stgmed_ld, FORMATETC *fmt)
+{
+    BYTE *data_def, *data_ld;
+    int i;
+    int data_size = 0, data_size_def, data_size_ld;
+
+    if (stgmed_def->tymed != stgmed_ld->tymed)
+        return E_FAIL;
+
+    if (fmt->cfFormat == CF_METAFILEPICT)
+    {
+        METAFILEPICT *mfpict_def = GlobalLock(U(stgmed_def)->hMetaFilePict);
+        METAFILEPICT *mfpict_ld = GlobalLock(U(stgmed_ld)->hMetaFilePict);
+
+        data_size_def = GetMetaFileBitsEx(mfpict_def->hMF, 0, NULL);
+        data_size_ld = GetMetaFileBitsEx(mfpict_ld->hMF, 0, NULL);
+        if (data_size_def == data_size_ld)
+        {
+            data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
+            data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
+            GetMetaFileBitsEx(mfpict_def->hMF, data_size_def, data_def);
+            GetMetaFileBitsEx(mfpict_ld->hMF, data_size_ld, data_ld);
+            data_size = data_size_def;
+        }
+        else return E_FAIL;
+    }
+    else if (fmt->cfFormat == CF_ENHMETAFILE)
+    {
+        data_size_def = GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, 0, NULL);
+        data_size_ld = GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, 0, NULL);
+        if (data_size_def == data_size_ld)
+        {
+            data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
+            data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
+            GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, data_size_def, data_def);
+            GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, data_size_ld, data_ld);
+            data_size = data_size_def;
+        }
+        else return E_FAIL;
+    }
+    else if (fmt->cfFormat == CF_DIB)
+    {
+        data_def = GlobalLock(stgmed_def->hGlobal);
+        data_ld = GlobalLock(stgmed_ld->hGlobal);
+        data_size = sizeof(dib);
+    }
+    else
+        return E_NOTIMPL;
+
+    for (i = 0; i < data_size; i++)
+    {
+        if (data_def[i] != data_ld[i]) return E_FAIL;
+    }
+
+    if (fmt->cfFormat == CF_DIB)
+    {
+        GlobalUnlock(stgmed_def->hGlobal);
+        GlobalUnlock(stgmed_ld->hGlobal);
+    }
+    else if (fmt->cfFormat == CF_METAFILEPICT)
+    {
+        GlobalUnlock(U(stgmed_def)->hMetaFilePict);
+        GlobalUnlock(U(stgmed_ld)->hMetaFilePict);
+    }
+
+    return S_OK;
+}
+
 static IStorage *create_storage_from_def(const struct storage_def *stg_def)
 {
     HRESULT hr;
@@ -4250,6 +4318,7 @@ static void test_data_cache_save_data(void)
     IStorage *doc;
     IOleCache2 *cache;
     IPersistStorage *persist;
+    IDataObject *odata;
     int enumerated_streams, matched_streams, i;
     DWORD dummy;
     struct tests_data_cache
@@ -4258,6 +4327,7 @@ static void test_data_cache_save_data(void)
         int num_fmts, num_set;
         const CLSID *clsid;
         struct storage_def stg_def;
+        STGMEDIUM stgmed_def[MAX_FMTS];
     };

static struct tests_data_cache *pdata, data[] =
@@ -4274,8 +4344,9 @@ static void test_data_cache_save_data(void)
                 &CLSID_WineTestOld, 4, { { "\2OlePres000", CF_DIB, DVASPECT_CONTENT, 0, NULL, 0 },
                                          { "\2OlePres001", CF_METAFILEPICT, DVASPECT_CONTENT, 0, NULL, 0 },
                                          { "\2OlePres002", CF_ENHMETAFILE, DVASPECT_CONTENT, 0, NULL, 0 },
-                                         { "\2OlePres003", 0, DVASPECT_DOCPRINT, 0, NULL, 0 } }
-            }
+                                         { "\2OlePres003", 0, DVASPECT_DOCPRINT, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         /* without setting data */
         {
@@ -4288,8 +4359,9 @@ static void test_data_cache_save_data(void)
             {
                 &CLSID_WineTestOld, 3, { { "\2OlePres000", CF_DIB, DVASPECT_CONTENT, 0, NULL, 0 },
                                          { "\2OlePres001", CF_METAFILEPICT, DVASPECT_CONTENT, 0, NULL, 0 },
-                                         { "\2OlePres002", CF_ENHMETAFILE, DVASPECT_CONTENT, 0, NULL, 0 } }
-            }
+                                         { "\2OlePres002", CF_ENHMETAFILE, DVASPECT_CONTENT, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         /* static picture clsids */
         {
@@ -4298,8 +4370,9 @@ static void test_data_cache_save_data(void)
             },
             1, 1, &CLSID_Picture_Dib,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         {
             {
@@ -4307,8 +4380,9 @@ static void test_data_cache_save_data(void)
             },
             1, 1, &CLSID_Picture_Metafile,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         {
             {
@@ -4316,8 +4390,9 @@ static void test_data_cache_save_data(void)
             },
             1, 1, &CLSID_Picture_EnhMetafile,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         /* static picture clsids without setting any data */
         {
@@ -4326,8 +4401,9 @@ static void test_data_cache_save_data(void)
             },
             1, 0, &CLSID_Picture_Dib,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         {
             {
@@ -4335,8 +4411,9 @@ static void test_data_cache_save_data(void)
             },
             1, 0, &CLSID_Picture_Metafile,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         {
             {
@@ -4344,8 +4421,9 @@ static void test_data_cache_save_data(void)
             },
             1, 0, &CLSID_Picture_EnhMetafile,
             {
-                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } }
-            }
+                &CLSID_WineTestOld, 1, { { "CONTENTS", -1, 0, 0, NULL, 0 } },
+            },
+            { { 0 } }
         },
         {
             {
@@ -4366,9 +4444,9 @@ static void test_data_cache_save_data(void)
             ok(SUCCEEDED(hr), "unexpected %#x\n", hr);
             if (i < pdata->num_set)
             {
-                get_stgmedium(pdata->fmts[i].cfFormat, &stgmed);
-                get_stgdef(&pdata->stg_def, pdata->fmts[i].cfFormat, &stgmed, i);
-                hr = IOleCache2_SetData(cache, &pdata->fmts[i], &stgmed, TRUE);
+                get_stgmedium(pdata->fmts[i].cfFormat, &pdata->stgmed_def[i]);
+                get_stgdef(&pdata->stg_def, pdata->fmts[i].cfFormat, &pdata->stgmed_def[i], i);
+                hr = IOleCache2_SetData(cache, &pdata->fmts[i], &pdata->stgmed_def[i], FALSE);
                 ok(hr == S_OK, "unexpected %#x\n", hr);
             }
         }
@@ -4399,12 +4477,59 @@ static void test_data_cache_save_data(void)
         ok(enumerated_streams == pdata->stg_def.stream_count, "created %d != def streams %d\n",
            enumerated_streams, pdata->stg_def.stream_count);

+        IPersistStorage_Release(persist);
+        IOleCache2_Release(cache);
+
+        /* now test _Load/_GetData using the storage we used for _Save */
+        hr = CreateDataCache(NULL, pdata->clsid, &IID_IOleCache2, (void **)&cache);
+        ok(hr == S_OK, "unexpected %#x\n", hr);
+        hr = IOleCache2_QueryInterface(cache, &IID_IPersistStorage, (void **)&persist);
+        ok(hr == S_OK, "unexpected %#x\n", hr);
+        for (i = 0; i < pdata->num_fmts; i++)
+        {
+            hr = IOleCache2_Cache(cache, &pdata->fmts[i], 0, &dummy);
+            ok(SUCCEEDED(hr), "unexpected %#x\n", hr);
+        }
+
+        if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Dib))
+        {
+            hr = IStorage_SetClass(doc, &CLSID_Picture_Dib);
+            ok(hr == S_OK, "unexpected %#x\n", hr);
+        }
+        else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Metafile))
+        {
+            hr = IStorage_SetClass(doc, &CLSID_Picture_Metafile);
+            ok(hr == S_OK, "unexpected %#x\n", hr);
+        }
+        else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_EnhMetafile))
+        {
+            hr = IStorage_SetClass(doc, &CLSID_Picture_EnhMetafile);
+            ok(hr == S_OK, "unexpected %#x\n", hr);
+        }
+        trace("IPersistStorage_Load\n");
+        hr = IPersistStorage_Load(persist, doc);
+        ok(hr == S_OK, "unexpected %#x\n", hr);
+
+        hr = IOleCache2_QueryInterface(cache, &IID_IDataObject, (void **)&odata);
+        ok(hr == S_OK, "unexpected %#x\n", hr);
         for (i = 0; i < pdata->num_set; i++)
-            HeapFree(GetProcessHeap(), 0, (void *)pdata->stg_def.stream[i].data);
+        {
+            hr = IDataObject_GetData(odata, &pdata->fmts[i], &stgmed);
+            ok(hr == S_OK, "unexpected %#x\n", hr);

+            hr = check_medium_contents(&pdata->stgmed_def[i], &stgmed, &pdata->fmts[i]);
+            ok(hr == S_OK, "unexpected %#x\n", hr);
+            ReleaseStgMedium(&stgmed);
+            ReleaseStgMedium(&pdata->stgmed_def[i]);
+        }
+
+        IDataObject_Release(odata);
         IPersistStorage_Release(persist);
         IStorage_Release(doc);
         IOleCache2_Release(cache);
+        for (i = 0; i < pdata->num_set; i++)
+            HeapFree(GetProcessHeap(), 0, (void *)pdata->stg_def.stream[i].data);
+
     }
 }

[PATCH v2 5/5] ole32/tests: Add tests for loading from presentation streams to data cache.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:37PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/tests/ole2.c | 165 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 145 insertions(+), 20 deletions(-)
>
> diff --git a/dlls/ole32/tests/ole2.c b/dlls/ole32/tests/ole2.c
> index 3bd4acce92..da9c5ff070 100644
> --- a/dlls/ole32/tests/ole2.c
> +++ b/dlls/ole32/tests/ole2.c
> @@ -4078,6 +4078,74 @@ static void check_storage_contents(IStorage *stg, const struct storage_def *stg_
>      }
>  }
>
> +static HRESULT check_medium_contents(const STGMEDIUM *stgmed_def, STGMEDIUM *stgmed_ld, FORMATETC *fmt)
> +{

Couldn't this be turned into a 'cmp' type function ie:
int stgmedium_cmp(const STGMEDIUM *s1, const STGMEDIUM s2)
? You don't actually need fmt since you can base the type selection on tymed.
> - BYTE *data_def, *data_ld;
> - int i;
> - int data_size = 0, data_size_def, data_size_ld;
> -
> - if (stgmed_def->tymed != stgmed_ld->tymed)
> -
>    return E_FAIL;
> -
> - if (fmt->cfFormat == CF_METAFILEPICT)
> - {
> -
>    METAFILEPICT *mfpict_def = GlobalLock(U(stgmed_def)->hMetaFilePict);
> -
>    METAFILEPICT *mfpict_ld = GlobalLock(U(stgmed_ld)->hMetaFilePict);
> -
> -
>    data_size_def = GetMetaFileBitsEx(mfpict_def->hMF, 0, NULL);
> -
>    data_size_ld = GetMetaFileBitsEx(mfpict_ld->hMF, 0, NULL);
> -
>    if (data_size_def == data_size_ld)
> -
>    {
> -
>        data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
> -
>        data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
> -
>        GetMetaFileBitsEx(mfpict_def->hMF, data_size_def, data_def);
> -
>        GetMetaFileBitsEx(mfpict_ld->hMF, data_size_ld, data_ld);
> -
>        data_size = data_size_def;
> -
>    }
> -
>    else return E_FAIL;
> - }
> - else if (fmt->cfFormat == CF_ENHMETAFILE)
> - {
> -
>    data_size_def = GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, 0, NULL);
> -
>    data_size_ld = GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, 0, NULL);
> -
>    if (data_size_def == data_size_ld)
> -
>    {
> -
>        data_def = HeapAlloc(GetProcessHeap(), 0, data_size_def);
> -
>        data_ld = HeapAlloc(GetProcessHeap(), 0, data_size_ld);
> -
>        GetEnhMetaFileBits(stgmed_def->hEnhMetaFile, data_size_def, data_def);
> -
>        GetEnhMetaFileBits(stgmed_ld->hEnhMetaFile, data_size_ld, data_ld);
> -
>        data_size = data_size_def;
> -
>    }
> -
>    else return E_FAIL;
> - }
> - else if (fmt->cfFormat == CF_DIB)
> - {
> -
>    data_def = GlobalLock(stgmed_def->hGlobal);
> -
>    data_ld = GlobalLock(stgmed_ld->hGlobal);
> -
>    data_size = sizeof(dib);
> - }
> - else
> -
>    return E_NOTIMPL;
> -
> - for (i = 0; i < data_size; i++)
> - {
> -
>    if (data_def[i] != data_ld[i]) return E_FAIL;

This is memcmp() right?
> - }
> -
> - if (fmt->cfFormat == CF_DIB)
> - {
> -
>    GlobalUnlock(stgmed_def->hGlobal);
> -
>    GlobalUnlock(stgmed_ld->hGlobal);
> - }
> - else if (fmt->cfFormat == CF_METAFILEPICT)
> - {
> -
>    GlobalUnlock(U(stgmed_def)->hMetaFilePict);
> -
>    GlobalUnlock(U(stgmed_ld)->hMetaFilePict);
> - }
> -
> - return S_OK;
>
> +}
> -
>
> static IStorage *create_storage_from_def(const struct storage_def *stg_def)
>  {
>      HRESULT hr;
> @@ -4250,6 +4318,7 @@ static void test_data_cache_save_data(void)

...
> @@ -4258,6 +4327,7 @@ static void test_data_cache_save_data(void)
>          int num_fmts, num_set;
>          const CLSID *clsid;
>          struct storage_def stg_def;
> -
>    STGMEDIUM stgmed_def[MAX_FMTS];

Do these really need to be part of this struct?  Couldn't they just be
a separate array?  Also, just call them meds[] or stgmeds[].
> @@ -4399,12 +4477,59 @@ static void test_data_cache_save_data(void)
>          ok(enumerated_streams == pdata->stg_def.stream_count, "created %d != def streams %d\n",
>             enumerated_streams, pdata->stg_def.stream_count);
> -
>    IPersistStorage_Release(persist);
> -
>    IOleCache2_Release(cache);
> -
> -
>    /* now test _Load/_GetData using the storage we used for _Save */
> -
>    hr = CreateDataCache(NULL, pdata->clsid, &IID_IOleCache2, (void **)&cache);
> -
>    ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    hr = IOleCache2_QueryInterface(cache, &IID_IPersistStorage, (void **)&persist);
> -
>    ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    for (i = 0; i < pdata->num_fmts; i++)
> -
>    {
> -
>        hr = IOleCache2_Cache(cache, &pdata->fmts[i], 0, &dummy);
> -
>        ok(SUCCEEDED(hr), "unexpected %#x\n", hr);
> -
>    }

In typical usage one wouldn't create the cache entries before a load.
> -
> -
>    if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Dib))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_Dib);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }
> -
>    else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_Metafile))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_Metafile);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }
> -
>    else if (IsEqualCLSID(pdata->clsid, &CLSID_Picture_EnhMetafile))
> -
>    {
> -
>        hr = IStorage_SetClass(doc, &CLSID_Picture_EnhMetafile);
> -
>        ok(hr == S_OK, "unexpected %#x\n", hr);
> -
>    }

I'm sure you can come up with a better way of setting the storage's class...
In fact, why not set it for all classes?

Huw.

[Huw Davies]

On Fri, Mar 30, 2018 at 12:11:33PM -0500, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/ole32/datacache.c | 185 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 122 insertions(+), 63 deletions(-)
>
> diff --git a/dlls/ole32/datacache.c b/dlls/ole32/datacache.c
> index 325a98b33b..799e14a867 100644
> --- a/dlls/ole32/datacache.c
> +++ b/dlls/ole32/datacache.c
> @@ -701,7 +764,62 @@ fail:
>      GlobalUnlock( hglobal );
>      GlobalFree( hglobal );
>      return E_FAIL;
> +}
>
> +static HRESULT load_emf( DataCacheEntry *cache_entry, IStream *stm )
> +{
> - HRESULT hr;
> -
> - if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> - {
> -
>    STGMEDIUM stgmed;
> -
> -
>    hr = load_mf_pict( cache_entry, stm );
> -
>    if (SUCCEEDED( hr ))
> -
>        hr = synthesize_emf( cache_entry->stgmedium.u.hMetaFilePict, &stgmed );
> -
>    if (SUCCEEDED( hr ))
> -
>    {
> -
>        ReleaseStgMedium( &cache_entry->stgmedium );

I think we want to release this even if synthesize_emf fails,
otherwise we'll be stuck with a mfpict in there, that will likely
confuse things later on.
> -
>        hr = copy_stg_medium( CF_ENHMETAFILE, &cache_entry->stgmedium, &stgmed );
> -
>        ReleaseStgMedium( &stgmed );

I didn't mean a deep copy here.  Just &cache_entry->stgmedium = stgmed; would do.
> -
>    }
> - }
> - else
> - {
> -
>    STATSTG stat;
> -
>    void *data;

If data were a BYTE * you won't need the cast later on.
> -
>    DWORD size_bits;
> -
>    ULONG read;
> -
> -
>    hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
> -
> -
>    if (SUCCEEDED( hr ))
> -
>    {
> -
>        data = HeapAlloc( GetProcessHeap(), 0, stat.cbSize.u.LowPart );
> -
>        if (!data) return E_OUTOFMEMORY;
> -
> -
>        hr = IStream_Read( stm, data, stat.cbSize.u.LowPart, &read );
> -
>        if (hr != S_OK || read != stat.cbSize.u.LowPart)
> -
>        {
> -
>            HeapFree( GetProcessHeap(), 0, data );
> -
>            return E_FAIL;
> -
>        }
> -
> -
>        size_bits = read - sizeof(DWORD) - sizeof(ENHMETAHEADER);
> -
>        if (size_bits <= 0)
> -
>        {
> -
>            HeapFree( GetProcessHeap(), 0, data );
> -
>            return E_FAIL;
> -
>        }
> -
>        cache_entry->stgmedium.u.hEnhMetaFile = SetEnhMetaFileBits( size_bits, (BYTE *)data + (read - size_bits) );
> -
>        cache_entry->stgmedium.tymed = TYMED_ENHMF;
> -
>        cache_entry->stgmedium.pUnkForRelease = NULL;
> -
> -
>        HeapFree( GetProcessHeap(), 0, data );
> -
>    }
> - }
> -
> - return hr;
>
> }