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@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 = {