[PATCH 0/2] MR582: winegstreamer: Prefer switch rather than misleading array indexes.

[Rémi Bernon]

Adding formats without updating the array gets unnoticed by the compiler otherwise.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/582

[PATCH 1/2] winegstreamer: Use a wg_audio_format switch rather than array indexes.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/winegstreamer/quartz_parser.c | 58 ++++++++++++++++++------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c
index 34848c0b503..eba18d60799 100644
--- a/dlls/winegstreamer/quartz_parser.c
+++ b/dlls/winegstreamer/quartz_parser.c
@@ -101,6 +101,40 @@ static HRESULT WINAPI GST_ChangeCurrent(IMediaSeeking *iface);
 static HRESULT WINAPI GST_ChangeStop(IMediaSeeking *iface);
 static HRESULT WINAPI GST_ChangeRate(IMediaSeeking *iface);
 
+static bool wg_audio_format_is_float(enum wg_audio_format format)
+{
+    switch (format)
+    {
+        case WG_AUDIO_FORMAT_UNKNOWN: return false;
+        case WG_AUDIO_FORMAT_U8: return false;
+        case WG_AUDIO_FORMAT_S16LE: return false;
+        case WG_AUDIO_FORMAT_S24LE: return false;
+        case WG_AUDIO_FORMAT_S32LE: return false;
+        case WG_AUDIO_FORMAT_F32LE: return true;
+        case WG_AUDIO_FORMAT_F64LE: return true;
+    }
+
+    assert(0);
+    return false;
+}
+
+static WORD wg_audio_format_get_depth(enum wg_audio_format format)
+{
+    switch (format)
+    {
+        case WG_AUDIO_FORMAT_UNKNOWN: return 0;
+        case WG_AUDIO_FORMAT_U8: return 8;
+        case WG_AUDIO_FORMAT_S16LE: return 16;
+        case WG_AUDIO_FORMAT_S24LE: return 24;
+        case WG_AUDIO_FORMAT_S32LE: return 32;
+        case WG_AUDIO_FORMAT_F32LE: return 32;
+        case WG_AUDIO_FORMAT_F64LE: return 64;
+    }
+
+    assert(0);
+    return 0;
+}
+
 static bool amt_from_wg_format_audio(AM_MEDIA_TYPE *mt, const struct wg_format *format)
 {
     mt->majortype = MEDIATYPE_Audio;
@@ -118,28 +152,8 @@ static bool amt_from_wg_format_audio(AM_MEDIA_TYPE *mt, const struct wg_format *
     case WG_AUDIO_FORMAT_F32LE:
     case WG_AUDIO_FORMAT_F64LE:
     {
-        static const struct
-        {
-            bool is_float;
-            WORD depth;
-        }
-        format_table[] =
-        {
-            {0},
-            {false, 8},
-            {false, 16},
-            {false, 24},
-            {false, 32},
-            {true, 32},
-            {true, 64},
-        };
-
-        bool is_float;
-        WORD depth;
-
-        assert(format->u.audio.format < ARRAY_SIZE(format_table));
-        is_float = format_table[format->u.audio.format].is_float;
-        depth = format_table[format->u.audio.format].depth;
+        bool is_float = wg_audio_format_is_float(format->u.audio.format);
+        WORD depth = wg_audio_format_get_depth(format->u.audio.format);
 
         if (is_float || format->u.audio.channels > 2)
         {
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/582

[PATCH 2/2] winegstreamer: Use a wg_video_format switch rather than array indexes.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/winegstreamer/quartz_parser.c | 100 +++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 27 deletions(-)

diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c
index eba18d60799..1ba24580cf6 100644
--- a/dlls/winegstreamer/quartz_parser.c
+++ b/dlls/winegstreamer/quartz_parser.c
@@ -373,32 +373,80 @@ unsigned int wg_format_get_max_size(const struct wg_format *format)
     return 0;
 }
 
-static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool wm)
+static const GUID *wg_video_format_get_mediasubtype(enum wg_video_format format)
 {
-    static const struct
+    switch (format)
     {
-        const GUID *subtype;
-        DWORD compression;
-        WORD depth;
+        case WG_VIDEO_FORMAT_UNKNOWN: return &GUID_NULL;
+        case WG_VIDEO_FORMAT_BGRA: return &MEDIASUBTYPE_ARGB32;
+        case WG_VIDEO_FORMAT_BGRx: return &MEDIASUBTYPE_RGB32;
+        case WG_VIDEO_FORMAT_BGR: return &MEDIASUBTYPE_RGB24;
+        case WG_VIDEO_FORMAT_RGB15: return &MEDIASUBTYPE_RGB555;
+        case WG_VIDEO_FORMAT_RGB16: return &MEDIASUBTYPE_RGB565;
+        case WG_VIDEO_FORMAT_AYUV: return &MEDIASUBTYPE_AYUV;
+        case WG_VIDEO_FORMAT_I420: return &MEDIASUBTYPE_I420;
+        case WG_VIDEO_FORMAT_NV12: return &MEDIASUBTYPE_NV12;
+        case WG_VIDEO_FORMAT_UYVY: return &MEDIASUBTYPE_UYVY;
+        case WG_VIDEO_FORMAT_YUY2: return &MEDIASUBTYPE_YUY2;
+        case WG_VIDEO_FORMAT_YV12: return &MEDIASUBTYPE_YV12;
+        case WG_VIDEO_FORMAT_YVYU: return &MEDIASUBTYPE_YVYU;
+        case WG_VIDEO_FORMAT_CINEPAK: return &MEDIASUBTYPE_CVID;
+    }
+
+    assert(0);
+    return NULL;
+}
+
+static DWORD wg_video_format_get_compression(enum wg_video_format format)
+{
+    switch (format)
+    {
+        case WG_VIDEO_FORMAT_UNKNOWN: return 0;
+        case WG_VIDEO_FORMAT_BGRA: return BI_RGB;
+        case WG_VIDEO_FORMAT_BGRx: return BI_RGB;
+        case WG_VIDEO_FORMAT_BGR: return BI_RGB;
+        case WG_VIDEO_FORMAT_RGB15: return BI_RGB;
+        case WG_VIDEO_FORMAT_RGB16: return BI_BITFIELDS;
+        case WG_VIDEO_FORMAT_AYUV: return mmioFOURCC('A','Y','U','V');
+        case WG_VIDEO_FORMAT_I420: return mmioFOURCC('I','4','2','0');
+        case WG_VIDEO_FORMAT_NV12: return mmioFOURCC('N','V','1','2');
+        case WG_VIDEO_FORMAT_UYVY: return mmioFOURCC('U','Y','V','Y');
+        case WG_VIDEO_FORMAT_YUY2: return mmioFOURCC('Y','U','Y','2');
+        case WG_VIDEO_FORMAT_YV12: return mmioFOURCC('Y','V','1','2');
+        case WG_VIDEO_FORMAT_YVYU: return mmioFOURCC('Y','V','Y','U');
+        case WG_VIDEO_FORMAT_CINEPAK: return mmioFOURCC('C','V','I','D');
     }
-    format_table[] =
-    {
-        {0},
-        {&MEDIASUBTYPE_ARGB32, BI_RGB,                      32},
-        {&MEDIASUBTYPE_RGB32,  BI_RGB,                      32},
-        {&MEDIASUBTYPE_RGB24,  BI_RGB,                      24},
-        {&MEDIASUBTYPE_RGB555, BI_RGB,                      16},
-        {&MEDIASUBTYPE_RGB565, BI_BITFIELDS,                16},
-        {&MEDIASUBTYPE_AYUV,   mmioFOURCC('A','Y','U','V'), 32},
-        {&MEDIASUBTYPE_I420,   mmioFOURCC('I','4','2','0'), 12},
-        {&MEDIASUBTYPE_NV12,   mmioFOURCC('N','V','1','2'), 12},
-        {&MEDIASUBTYPE_UYVY,   mmioFOURCC('U','Y','V','Y'), 16},
-        {&MEDIASUBTYPE_YUY2,   mmioFOURCC('Y','U','Y','2'), 16},
-        {&MEDIASUBTYPE_YV12,   mmioFOURCC('Y','V','1','2'), 12},
-        {&MEDIASUBTYPE_YVYU,   mmioFOURCC('Y','V','Y','U'), 16},
-        {&MEDIASUBTYPE_CVID,   mmioFOURCC('C','V','I','D'), 24},
-    };
 
+    assert(0);
+    return 0;
+}
+
+static WORD wg_video_format_get_depth(enum wg_video_format format)
+{
+    switch (format)
+    {
+        case WG_VIDEO_FORMAT_UNKNOWN: return 0;
+        case WG_VIDEO_FORMAT_BGRA: return 32;
+        case WG_VIDEO_FORMAT_BGRx: return 32;
+        case WG_VIDEO_FORMAT_BGR: return 24;
+        case WG_VIDEO_FORMAT_RGB15: return 16;
+        case WG_VIDEO_FORMAT_RGB16: return 16;
+        case WG_VIDEO_FORMAT_AYUV: return 32;
+        case WG_VIDEO_FORMAT_I420: return 12;
+        case WG_VIDEO_FORMAT_NV12: return 12;
+        case WG_VIDEO_FORMAT_UYVY: return 16;
+        case WG_VIDEO_FORMAT_YUY2: return 16;
+        case WG_VIDEO_FORMAT_YV12: return 12;
+        case WG_VIDEO_FORMAT_YVYU: return 16;
+        case WG_VIDEO_FORMAT_CINEPAK: return 24;
+    }
+
+    assert(0);
+    return 0;
+}
+
+static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool wm)
+{
     VIDEOINFO *video_format;
     uint32_t frame_time;
 
@@ -408,10 +456,8 @@ static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format *
     if (!(video_format = CoTaskMemAlloc(sizeof(*video_format))))
         return false;
 
-    assert(format->u.video.format < ARRAY_SIZE(format_table));
-
     mt->majortype = MEDIATYPE_Video;
-    mt->subtype = *format_table[format->u.video.format].subtype;
+    mt->subtype = *wg_video_format_get_mediasubtype(format->u.video.format);
     if (wm)
         mt->bFixedSizeSamples = TRUE;
     else
@@ -434,8 +480,8 @@ static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format *
     video_format->bmiHeader.biWidth = format->u.video.width;
     video_format->bmiHeader.biHeight = format->u.video.height;
     video_format->bmiHeader.biPlanes = 1;
-    video_format->bmiHeader.biBitCount = format_table[format->u.video.format].depth;
-    video_format->bmiHeader.biCompression = format_table[format->u.video.format].compression;
+    video_format->bmiHeader.biBitCount = wg_video_format_get_depth(format->u.video.format);
+    video_format->bmiHeader.biCompression = wg_video_format_get_compression(format->u.video.format);
     video_format->bmiHeader.biSizeImage = wg_format_get_max_size(format);
 
     if (format->u.video.format == WG_VIDEO_FORMAT_RGB16)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/582

[Rémi Bernon]

Is there anything I should do here?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/582#note_6130

[Zebediah Figura (she/her)]

On 8/12/22 01:51, Rémi Bernon (@rbernon) wrote:
> Is there anything I should do here?

Sorry, I just accidentally dropped this on the floor.

I'm not thrilled about how much more space this requires, but I don't 
see any better way to do these checks at compile time, so I'll approve 
the merge request.

[Gabriel Ivăncescu]

On 12/08/2022 19:57, Zebediah Figura (she/her) wrote:
> On 8/12/22 01:51, Rémi Bernon (@rbernon) wrote:
> > Is there anything I should do here?
>
> Sorry, I just accidentally dropped this on the floor.
>
> I'm not thrilled about how much more space this requires, but I don't 
> see any better way to do these checks at compile time, so I'll approve 
> the merge request.
> _______________________________________________
> wine-gitlab mailing list -- wine-gitlab(a)winehq.org
> To unsubscribe send an email to wine-gitlab-leave(a)winehq.org

FWIW, you can use C_ASSERT near the array and add some enum value at the 
end that marks the count of formats in the enum (WG_AUDIO_FORMAT_NUM?). 
It's used in some other parts of wine.

[Zebediah Figura (she/her)]

On 8/12/22 14:17, Gabriel Ivăncescu wrote:
> On 12/08/2022 19:57, Zebediah Figura (she/her) wrote:
> > On 8/12/22 01:51, Rémi Bernon (@rbernon) wrote:
> > > Is there anything I should do here?
> >
> > Sorry, I just accidentally dropped this on the floor.
> >
> > I'm not thrilled about how much more space this requires, but I don't 
> > see any better way to do these checks at compile time, so I'll approve 
> > the merge request.
> > _______________________________________________
> > wine-gitlab mailing list -- wine-gitlab(a)winehq.org
> > To unsubscribe send an email to wine-gitlab-leave(a)winehq.org
>
> FWIW, you can use C_ASSERT near the array and add some enum value at the 
> end that marks the count of formats in the enum (WG_AUDIO_FORMAT_NUM?). 
> It's used in some other parts of wine.
> _______________________________________________
> wine-gitlab mailing list -- wine-gitlab(a)winehq.org
> To unsubscribe send an email to wine-gitlab-leave(a)winehq.org

It works, although I'm still not sure I like it better than this approach.

I'd prefer to use designated initializers, but that won't catch missing 
entries at compile time.

[PATCH 0/2] MR582: winegstreamer: Prefer switch rather than misleading array indexes. - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/582