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.

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

Correct JavaFxPlayVideoAndAudio sample to synchronize audio and video… #1662

Merged
merged 5 commits into from
Jun 21, 2021

Conversation

jpsacha
Copy link
Member

@jpsacha jpsacha commented Jun 13, 2021

… and prevent "dropping" of video frames

Before correction, video frames may be displayed in a quick succession resulting in a impression of dropped frames. Now video frames are synced to the timestamp of audio playback.

… and prevent "dropping" of video frames

Before correction, video frames may be displeyed in a quick successtion resulting in a impression of dropped frames. NOw vodeo frames are synced to the timestamp of audio playback.
@saudet
Copy link
Member

saudet commented Jun 14, 2021

Do we really need a new flag? It doesn't work with Thread.interrupt()? Also, I was under the impression that executor.submit(...).get() did the equivalent of Thread.sleep(). That's not the case? @d-a-gerashenko Would you know why it's not working?

@d-a-gerashenko
Copy link
Contributor

d-a-gerashenko commented Jun 14, 2021

  1. I don't see audioFormat initialization, i.e. "javax.sound.sampled.AudioFormat audioFormat = new AudioFormat(8000, 16, 1, true, true);" The cause of desync could be an incorrect audio format settings.
  2. requestPlayThreadStop isn't better in this case because it's just a demo.
  3. "Also, I was under the impression that executor.submit(...).get() did the equivalent of Thread.sleep()." it's right. The main thread waits for end of submitted task who plays sound sample. I don't know if there is a case when a frequency of images is greater than a frequency of sound samples (in this case images could be shown too fast).

@jpsacha
Copy link
Member Author

jpsacha commented Jun 14, 2021

@saudet a flag is used as a different way of closing the application. After addition of Thread.sleep to correctly control the frame rate additional code is needed do deal with interrupt() (issued in stop()). Using a flag and stopping without a interrupt simplifies the overall example. May be a matter of preference, looks simpler to me.

I removed get() from submit because it blocks display of images till audio is played. This prevents display of image frames that corresponds to that audio at correct timestamps. Note that audio and image frames do not have to be interleaved 1:1 when decoding. There may be a couple of audio frames followed by a couple of image frames. In the original example, the audio will play almost correctly, but image frames were displayed incorrectly. They can bunch-up, if they arrive without exactly interleaving audio frames, they are displayed almost at the same time resulting in choppy video. This is very noticeable when playing a video of a speaker on lower frame rates. May depend on how particular video was encoded. I have some examples were it looked very choppy.

In the corrected code, the audio is written/played in a concurrent thread allowing display of corresponding images. The soundLine is responsible for audio playback, we just need to add the samples to play. Since soundLine provides information about currently played timestamp we can sync images to the audio using Thread.sleep()

@d-a-gerashenko 1. Audio initialization did not change, it is still there, about line 61.
2. Nothing against interrupt. Flag requestPlayThreadStop seems simpler in this case. After adding Thread.sleep that can be interrupted by stop() issuing exceptions when, we will need to add exception handling there.
3. As explained above. The problem with get() is that it does not account for situations when image and sound frames are not interleaved 1:1. Also using 'get()' adds small brakes to sound playback when images are grabbed and displayed.

The original example nicely shows how to decode audio and play video with sound. The correction is just to improve video/image display and use JavaFXFrameConverter.

@d-a-gerashenko
Copy link
Contributor

@jpsacha
I didn't remember why I decided to use interrupt but I spouse that it's because of grab. On exit we should immediately exit without waiting for grab.

And what about sleep I see only one case I'm not sure about. Could the sleep() became the cause of gaps between sound samples? Shouldn't we use here another one thread for image displaying in time?

@jpsacha
Copy link
Member Author

jpsacha commented Jun 14, 2021

@d-a-gerashenko Stopping a thread with a flag assumes that the flag is checked frequently. When we are playing video it should happen many times within a second (at least at the frame rate). If a stop has to be forced quicker am interrupt() may be better choice. If you think it is really necessary here I can put it back.

Can Thread.sleep() cause breaks in the audio playback? If we do not keep the buffer of soundLine filled up it could theoretically happen. I played a couple of videos and did not observed that. I used soundLine.addLineListener to monitor events:

soundLine.addLineListener(new LineListener() {
    public void update(LineEvent event) {
        System.out.println("Line Event: "+ event.toString());
    }
});

If the audio playback stops, for instance when buffer gets empty, there will be a STOP event. I only see it when video finishes.
Note that after Thread.sleep happens an image is displayed on a separate JavaFX thread (not on playThread). I also looked at timestamps for grabbed image, grabbed audio, and playback timestamps in a couple of videos. Here is a sample for a couple of frames

Grabbed audio timestamp: 8382403, soundLine timestamp:7980748
Grabbed audio timestamp: 8405623, soundLine timestamp:7980748
Grabbed image timestamp: 8008000, soundLine timestamp:7980748, delay: 27252
Grabbed audio timestamp: 8428843, soundLine timestamp:8010748
Grabbed image timestamp: 8041366, soundLine timestamp:8010748, delay: 30618
Grabbed audio timestamp: 8452063, soundLine timestamp:8040748

Timestamps for grabbed audio frames are ahead of grabbed image frames which allows for keeping the soundLine buffer filled up. So while we could add an executor for converting images I do not see an evidence that it is necessary, at least in videos that I tried.

@d-a-gerashenko
Copy link
Contributor

d-a-gerashenko commented Jun 15, 2021

Finally:

  1. interrupt() is needed for immediately exit from grabber.grab().
  2. The behavior may be different for video from a file and streaming video (i.e. RTSP cams or https://test-videos.co.uk/bigbuckbunny/mp4-h264). But if new approach works fine for both of them than that's ok.

1) Bring back "Thread.interrupt()"
2) Use "executor" for image branch too.
3) Enable playing videos without sound, like https://test-videos.co.uk/bigbuckbunny/mp4-h264
4) Correctly play sound in http://download.oracle.com/otndocs/products/javafx/oow2010-2.flv
@jpsacha
Copy link
Member Author

jpsacha commented Jun 16, 2021

interrupt() is back and a couple of other improvement were added

@saudet saudet merged commit 216f51b into bytedeco:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants