[PATCH] dmime: Parse lyric track data

[Michael Stefaniuc]

Hello Alistair,

thanks for the patch.
parse_lyrics_track_events() and parse_lyricstrack_list() are so similar
that I had to diff them to see the difference.

Looking at the LyricsTrack documentation
https://docs.microsoft.com/en-us/previous-versions/ms810679%28v%3dmsdn.10%29
the parse_lyrics_track_events() is the actual event. The 'lyre' LIST is
used as a struct for the (single) event.

The beauty of the chunk helpers is that we don't have to do the
while-switch orgy like the old way.
In the LyricsTrack the number and order of the chunks is well defined.
So I would merge parse_lyrics_track_events() into
parse_lyrics_track_event() while dropping the while and switch.
Something like this shortened pseudo-code:
  stream_next_chunk()
  if (!FOURCC_LIST || !DMUS_FOURCC_LYRICSTRACKEVENT_LIST)
      return FAIL
  stream_next_chunk()
  if (!DMUS_FOURCC_LYRICSTRACKEVENTHEADER_CHUNK)
      return FAIL
  stream_chunk_get_data()
  stream_next_chunk()
  if (!DMUS_FOURCC_LYRICSTRACKEVENTTEXT_CHUNK)
      return FAIL
  stream_chunk_get_data()



Btw. you have two TRACEs with %g in them and I'm getting warnings for those:
dlls/dmime/lyricstrack.c:265:23: warning: format '%g' expects argument
of type 'double', but argument 5 has type 'MUSIC_TIME' {aka 'int'}
[-Wformat=]
dlls/dmime/lyricstrack.c:266:23: warning: format '%g' expects argument
of type 'double', but argument 5 has type 'MUSIC_TIME' {aka 'int'}
[-Wformat=]


Oh, can you also please downgrade the FIXME in parse_lyricstrack_list()
to a TRACE or WARN?
No other chunk is expected there so we can silently skip a stray other
chunk.


thanks
bye
	michael


On 4/23/20 7:20 AM, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair at hotmail.com>
> ---
>  dlls/dmime/lyricstrack.c | 121 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
> 
> diff --git a/dlls/dmime/lyricstrack.c b/dlls/dmime/lyricstrack.c
> index 49f44f52a34..17d3c3f26c4 100644
> --- a/dlls/dmime/lyricstrack.c
> +++ b/dlls/dmime/lyricstrack.c
> @@ -37,6 +37,11 @@ static inline IDirectMusicLyricsTrack *impl_from_IDirectMusicTrack8(IDirectMusic
>      return CONTAINING_RECORD(iface, IDirectMusicLyricsTrack, IDirectMusicTrack8_iface);
>  }
>  
> +static inline IDirectMusicLyricsTrack *impl_from_IPersistStream(IPersistStream *iface)
> +{
> +    return CONTAINING_RECORD(iface, IDirectMusicLyricsTrack, dmobj.IPersistStream_iface);
> +}
> +
>  static HRESULT WINAPI lyrics_track_QueryInterface(IDirectMusicTrack8 *iface, REFIID riid,
>          void **ret_iface)
>  {
> @@ -236,10 +241,122 @@ static const IDirectMusicTrack8Vtbl dmtrack8_vtbl = {
>      lyrics_track_Join
>  };
>  
> +static HRESULT parse_lyrics_track_event(IDirectMusicLyricsTrack *This, IStream *stream, struct chunk_entry *lyric)
> +{
> +    HRESULT hr;
> +    struct chunk_entry chunk = {.parent = lyric};
> +
> +    TRACE("Parsing segment form in %p: %s\n", stream, debugstr_chunk(lyric));
> +
> +    while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {
> +        switch (chunk.id) {
> +            case DMUS_FOURCC_LYRICSTRACKEVENTHEADER_CHUNK:
> +            {
> +                DMUS_IO_LYRICSTRACK_EVENTHEADER header;
> +
> +                if (FAILED(hr = stream_chunk_get_data(stream, &chunk, &header, chunk.size))) {
> +                    WARN("Failed to read data of %s\n", debugstr_chunk(&chunk));
> +                    return hr;
> +                }
> +
> +                TRACE("Found DMUS_IO_LYRICSTRACK_EVENTHEADER\n");
> +                TRACE("    - dwFlags 0x%08x\n", header.dwFlags);
> +                TRACE("    - dwTimingFlags 0x%08x\n", header.dwTimingFlags);
> +                TRACE("    - lTimeLogical %g\n", header.lTimeLogical);
> +                TRACE("    - lTimePhysical %g\n", header.lTimePhysical);
> +                break;
> +            }
> +            case DMUS_FOURCC_LYRICSTRACKEVENTTEXT_CHUNK:
> +            {
> +                WCHAR name[256];
> +
> +                if (FAILED(hr = stream_chunk_get_data(stream, &chunk, &name, chunk.size))) {
> +                    WARN("Failed to read data of %s\n", debugstr_chunk(&chunk));
> +                    return hr;
> +                }
> +
> +                TRACE("Found DMUS_FOURCC_LYRICSTRACKEVENTTEXT_CHUNK\n");
> +                TRACE("    - name %s\n", debugstr_w(name));
> +
> +                break;
> +            }
> +            default:
> +                FIXME("Parsing %s\n", debugstr_fourcc(chunk.id));
> +        }
> +    }
> +
> +    return SUCCEEDED(hr) ? S_OK : hr;
> +}
> +
> +static HRESULT parse_lyrics_track_events(IDirectMusicLyricsTrack *This, IStream *stream, struct chunk_entry *lyric)
> +{
> +    HRESULT hr;
> +    struct chunk_entry chunk = {.parent = lyric};
> +
> +    TRACE("Parsing segment form in %p: %s\n", stream, debugstr_chunk(lyric));
> +
> +    while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {
> +        switch (chunk.id) {
> +             case FOURCC_LIST:
> +                if (chunk.type == DMUS_FOURCC_LYRICSTRACKEVENT_LIST) {
> +                    if (FAILED(hr = parse_lyrics_track_event(This, stream, &chunk)))
> +                        return hr;
> +                }
> +                break;
> +             default:
> +                FIXME("Parsing %s\n", debugstr_fourcc(chunk.id));
> +        }
> +    }
> +
> +    return SUCCEEDED(hr) ? S_OK : hr;
> +}
> +
> +static HRESULT parse_lyricstrack_list(IDirectMusicLyricsTrack *This, IStream *stream, struct chunk_entry *lyric)
> +{
> +    HRESULT hr;
> +    struct chunk_entry chunk = {.parent = lyric};
> +
> +    TRACE("Parsing segment form in %p: %s\n", stream, debugstr_chunk(lyric));
> +
> +    while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {
> +        switch (chunk.id) {
> +            case FOURCC_LIST:
> +                if (chunk.type == DMUS_FOURCC_LYRICSTRACKEVENTS_LIST) {
> +                    if (FAILED(hr = parse_lyrics_track_events(This, stream, &chunk)))
> +                        return hr;
> +                }
> +                break;
> +            default:
> +                FIXME("Parsing %s\n", debugstr_fourcc(chunk.id));
> +        }
> +    }
> +
> +    return SUCCEEDED(hr) ? S_OK : hr;
> +}
> +
>  static HRESULT WINAPI lyrics_IPersistStream_Load(IPersistStream *iface, IStream *stream)
>  {
> -	FIXME(": Loading not implemented yet\n");
> -	return S_OK;
> +    IDirectMusicLyricsTrack *This = impl_from_IPersistStream(iface);
> +    HRESULT hr;
> +    struct chunk_entry chunk = {0};
> +
> +    TRACE("%p, %p\n", This, stream);
> +
> +    if (!stream)
> +        return E_POINTER;
> +
> +    if ((hr = stream_get_chunk(stream, &chunk) != S_OK))
> +        return hr;
> +
> +    if (chunk.id == FOURCC_LIST && chunk.type == DMUS_FOURCC_LYRICSTRACK_LIST) {
> +        hr = parse_lyricstrack_list(This, stream, &chunk);
> +    }
> +    else {
> +        FIXME("Unsupported id %s\n", debugstr_fourcc (chunk.id));
> +        hr = DMUS_E_UNSUPPORTED_STREAM;
> +    }
> +
> +	return hr;
>  }
>  
>  static const IPersistStreamVtbl persiststream_vtbl = {
>