Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Seeking forward in MIDI files doesn't resend PROGRAM CHANGE events #704

Closed
loki666 opened this issue Oct 6, 2023 · 6 comments
Closed

Seeking forward in MIDI files doesn't resend PROGRAM CHANGE events #704

loki666 opened this issue Oct 6, 2023 · 6 comments
Assignees
@christosts
Labels
bug

Comments

@loki666
Copy link
Contributor

loki666 commented Oct 6, 2023 •
edited

Version

Media3 pre-release (alpha, beta or RC not in this list)

More version details

release-1.2.0-alpha01

Devices that reproduce the issue

Not device specific, so emulator, Pixel etc...

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Yes

Reproduction steps

Using demo app with MidiExtract and MidiRenderer
Open a MIDI file
Seek forward in the file, and all channels are reset to default program (usually Piano)
Seek backward in the file, and all channels are properly configured back.

I've put some logs, and it seems indeed, that when seeking forward, MidiDecoder reset the Synthesizer but never sends the PROGRAM CHANGES events, but does so when seeking backward.

Expected result

MIDI channels are properly configured back after seeking forward

Actual result

MIDI channels are reset to default program (Piano) after seeking forward

Media

Not applicable, any MIDI file should expose the issue
@christosts christosts assigned christosts and unassigned rohitjoins Oct 19, 2023
@christosts
Copy link
Contributor

Can you please share a specific MIDI file that we can reproduce the issue? I've tried several MIDI files but can't repro. I hear all the instruments regardless of seeking forwards or backwards.

Thanks

@loki666
Copy link
Contributor Author

loki666 commented Nov 14, 2023

midi_tests.zip

Here it is,
For Doom midi, there is a overrdrive guitar that plays the "melody", which clearly switch to Piano when seeking
For scabbmap, the melody is a flute like instrument

I made some more tests, and sometimes it works when seeking forward, and sometimes seeking backward also revert to piano.

@loki666
Copy link
Contributor Author

loki666 commented Nov 15, 2023

to check the behavior,

add a logging function in MidiDecoder

    private void logMidiMessage(byte[] data, int offset, int count) {
        StringBuilder text = new StringBuilder("Received: ");
        if ((data[0] & 0xf0) == 0xc0) {
            text.append("PGM CHG ");
        }
        for (int i = 0; i < count; i++) {
            text.append(String.format("0x%02X, ", data[offset + i]));
        }
        Log.i("MidiDecoder", text.toString());
    }

and call it before (or after)

media/libraries/decoder_midi/src/main/java/androidx/media3/decoder/midi/MidiDecoder.java

Line 163 in 35219ee

midiSynthesizer.onReceive(

add a log in resetSynthesizers()

you clearly see that PGM CHG events are not sent when seeking forward

@christosts
Copy link
Contributor

thank you, I can reproduce this, we'll work on a fix

copybara-service bot pushed a commit that referenced this issue Nov 21, 2023
@christosts
MidiExtractor: mark only the first sample as key-frame
ec08db4 
This change fixes a bug with seeking forward in MIDI. When seeking forward,
the progressive media period attempts to seek within the sample queue, if a
key-frame exists before the seeking position. With MIDI, however, we can
only skip Note-On and Note-Off samples and all other samples must be sent
to the MIDI decoder.

When seeking outside the sample queue, the MidiExtractor already
instructs the player to start from the beginning of the MIDI input. With
this change, only the first output sample is a key-frame, thus the
progressive media period can no longer seek within the sample queue and
is forced to seek from the MIDI input start always.

Issue: #704

#minor-release

PiperOrigin-RevId: 584321443
copybara-service bot pushed a commit to google/ExoPlayer that referenced this issue Nov 21, 2023
@christosts
MidiExtractor: mark only the first sample as key-frame
61ebeab 
This change fixes a bug with seeking forward in MIDI. When seeking forward,
the progressive media period attempts to seek within the sample queue, if a
key-frame exists before the seeking position. With MIDI, however, we can
only skip Note-On and Note-Off samples and all other samples must be sent
to the MIDI decoder.

When seeking outside the sample queue, the MidiExtractor already
instructs the player to start from the beginning of the MIDI input. With
this change, only the first output sample is a key-frame, thus the
progressive media period can no longer seek within the sample queue and
is forced to seek from the MIDI input start always.

Issue: androidx/media#704

#minor-release

PiperOrigin-RevId: 584321443
@christosts
Copy link
Contributor

the linked commit should fix this. Would you be able to checkout the main branch and give it a try too?

@loki666
Copy link
Contributor Author

loki666 commented Nov 22, 2023

seems to do the trick !
thanks

@loki666 loki666 closed this as completed Nov 22, 2023
microkatz pushed a commit that referenced this issue Jan 11, 2024
@christosts @microkatz
MidiExtractor: mark only the first sample as key-frame
124eb31 
This change fixes a bug with seeking forward in MIDI. When seeking forward,
the progressive media period attempts to seek within the sample queue, if a
key-frame exists before the seeking position. With MIDI, however, we can
only skip Note-On and Note-Off samples and all other samples must be sent
to the MIDI decoder.

When seeking outside the sample queue, the MidiExtractor already
instructs the player to start from the beginning of the MIDI input. With
this change, only the first output sample is a key-frame, thus the
progressive media period can no longer seek within the sample queue and
is forced to seek from the MIDI input start always.

Issue: #704

PiperOrigin-RevId: 584321443
(cherry picked from commit ec08db4)
@androidx androidx locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@christosts christosts

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@loki666 @google-oss-bot @rohitjoins @christosts