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

WebM Movie Player (VP9/Opus) #458

Merged
merged 16 commits into from Dec 26, 2014

Conversation

Projects
None yet
6 participants
@Mystler
Contributor

Mystler commented Oct 29, 2014

Now, this is what I call a collaboration.
This, finally, implements our movie player. It can play webm containers that include vp9 video and opus audio. As a bonus, it supports multiple tracks and tries to select the track that matches the game language, if possible.

@Mystler Mystler referenced this pull request Oct 29, 2014

Merged

WebM movies #89

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
// TODO: Figure out video and audio based on current language
// For now... just take the first one.
const mkvparser::Tracks* tracks = fSegment->GetTracks();
for (uint32_t i = 0; i < tracks->GetTracksCount(); ++i)

This comment has been minimized.

@branan

branan Oct 29, 2014

Member

Just so I can make sure I understand this: This will use the first audio and video tracks, UNLESS a later one happens to match the current language?

@branan

branan Oct 29, 2014

Member

Just so I can make sure I understand this: This will use the first audio and video tracks, UNLESS a later one happens to match the current language?

This comment has been minimized.

@Mystler

Mystler Oct 29, 2014

Contributor

Correct.

@Mystler

Mystler Oct 29, 2014

Contributor

Correct.

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
SAFE_OP(seg->Load(), "load segment from webm");
fSegment.reset(seg);
// TODO: Figure out video and audio based on current language

This comment has been minimized.

@branan

branan Oct 29, 2014

Member

This comment is out of date

@branan

branan Oct 29, 2014

Member

This comment is out of date

This comment has been minimized.

@Mystler

Mystler Oct 29, 2014

Contributor

Thanks for catching that. Updated.

@Mystler

Mystler Oct 29, 2014

Contributor

Thanks for catching that. Updated.

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
VPX() { }
public:
#ifdef VIDEO_AVAILABLE

This comment has been minimized.

@dpogue

dpogue Oct 30, 2014

Member

Should this ifdef be one line higher? Currently you'd end up with the following if VIDEO_AVAILABLE were not defined:

class VPX
{
    VPX() { }

public:
}
@dpogue

dpogue Oct 30, 2014

Member

Should this ifdef be one line higher? Currently you'd end up with the following if VIDEO_AVAILABLE were not defined:

class VPX
{
    VPX() { }

public:
}
@Mystler

This comment has been minimized.

Show comment
Hide comment
@Mystler

Mystler Oct 30, 2014

Contributor

Moved that ifdef. Also, the last libwebm release is over a year old, so I took the latest files from master and updated the TrackMgr to respect the CodecDelay (libwebm supports that now, see mkv specs). This means that our track synchronization should be a few ms more exact, now. The specific change is a one-liner squashed into 2d990ae.

Contributor

Mystler commented Oct 30, 2014

Moved that ifdef. Also, the last libwebm release is over a year old, so I took the latest files from master and updated the TrackMgr to respect the CodecDelay (libwebm supports that now, see mkv specs). This means that our track synchronization should be a few ms more exact, now. The specific change is a one-liner squashed into 2d990ae.

Mystler added some commits Oct 26, 2014

Make audio work
Includes some reorganization and cleanup
Calculate block alignment properly
*sigh* This happens when you copy the formula from somewhere else... Let's do it correctly now and fix the source in plVoiceChat as well.
Rewrite TrackMgr
Also:
Do not start the video again if it is already playing.
Update files from libwebm
Implement Pause
Also, do not show the plate before there is anything on it.
Fix video size
Scale plate using pixels and limit it to viewport size
@Mystler

This comment has been minimized.

Show comment
Hide comment
@Mystler

Mystler Oct 31, 2014

Contributor

As I mentioned in the IRC, it looks like URU sometimes doesn't like it if we decode our audio frames on-demand. I added a new commit that changes it to preload them when we start the video. I haven't experienced any noticeable delay with this change and it fixes old record player like crackling problems in release mode.

Contributor

Mystler commented Oct 31, 2014

As I mentioned in the IRC, it looks like URU sometimes doesn't like it if we decode our audio frames on-demand. I added a new commit that changes it to preload them when we start the video. I haven't experienced any noticeable delay with this change and it fixes old record player like crackling problems in release mode.

if (!(mov->GetCmd() & plMovieMsg::kMake))
{
for (i = 0; i < fMovies.GetCount(); i++)
for (i = 0; i < fMovies.size(); i++)

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

It would be super awesome if this were a range-based for instead

@Hoikas

Hoikas Nov 1, 2014

Member

It would be super awesome if this were a range-based for instead

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

Uhm... i is used outside the loop, so I didn't touch it.

@Mystler

Mystler Nov 1, 2014

Contributor

Uhm... i is used outside the loop, so I didn't touch it.

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

Hmm... crazy code. This can be addressed later.

@Hoikas

Hoikas Nov 1, 2014

Member

Hmm... crazy code. This can be addressed later.

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
@@ -42,25 +42,417 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
#include "plMoviePlayer.h"
#ifdef VIDEO_AVAILABLE

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

This is kind of ambiguous--are we referring to vpx+opus or just vpx?

@Hoikas

Hoikas Nov 1, 2014

Member

This is kind of ambiguous--are we referring to vpx+opus or just vpx?

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

In your original version it was VPX_AVAILABLE which I found misleading. VIDEO should represent both libs, but I can change it to MOVIE_AVAILABLE, if you prefer that.

@Mystler

Mystler Nov 1, 2014

Contributor

In your original version it was VPX_AVAILABLE which I found misleading. VIDEO should represent both libs, but I can change it to MOVIE_AVAILABLE, if you prefer that.

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
public:
vpx_codec_ctx_t codec;
~VPX() {

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

code style nitpick

@Hoikas

Hoikas Nov 1, 2014

Member

code style nitpick

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

Guess who wrote that. :p

@Mystler

Mystler Nov 1, 2014

Contributor

Guess who wrote that. :p

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
for (int32_t i = 0; i < block->GetFrameCount(); i++)
{
const mkvparser::Block::Frame data = block->GetFrame(i);
uint8_t* buf = new uint8_t[data.len];

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

potential optimization: move the allocation step out of the loop and only (re-)allocate if the existing buffer is not big enough

@Hoikas

Hoikas Nov 1, 2014

Member

potential optimization: move the allocation step out of the loop and only (re-)allocate if the existing buffer is not big enough

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

I could use a std::vector to do that, I assume.

@Mystler

Mystler Nov 1, 2014

Contributor

I could use a std::vector to do that, I assume.

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

Actually, I'm not sure how to approach this, since that buffer is going into the frames container as a unique pointer.

@Mystler

Mystler Nov 1, 2014

Contributor

Actually, I'm not sure how to approach this, since that buffer is going into the frames container as a unique pointer.

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

Oh, I thought this was a buffer that did not have a long life span. Whoops!

@Hoikas

Hoikas Nov 1, 2014

Member

Oh, I thought this was a buffer that did not have a long life span. Whoops!

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
std::vector<int16_t> decoded;
decoded.reserve(frames.size() * audio->GetChannels() * maxFrameSize);
for (auto it = frames.begin(); it != frames.end(); ++it)

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

range based for would be super awesome

@Hoikas

Hoikas Nov 1, 2014

Member

range based for would be super awesome

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
const std::unique_ptr<uint8_t>& buf = std::get<0>(*it);
int32_t size = std::get<1>(*it);
int16_t* pcm = new int16_t[maxFrameSize * audio->GetChannels()];

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

we could optimize this by moving the allocation outside of the loop. (heap alloc is slow)

@Hoikas

Hoikas Nov 1, 2014

Member

we could optimize this by moving the allocation outside of the loop. (heap alloc is slow)

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
vpx_image_t* img = nullptr;
// We have to decode all the frames, but we only want to display the most recent one to the user.
for (auto it = frames.begin(); it != frames.end(); ++it)

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

range based for?

@Hoikas

Hoikas Nov 1, 2014

Member

range based for?

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
const mkvparser::VideoTrack* video = static_cast<const mkvparser::VideoTrack*>(fVideoTrack->GetTrack());
if (strcmp(video->GetCodecId(), WEBM_CODECID_VP9) != 0)
{
hsAssert(false, "Not a VP9 video track!");

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

this should probably be a log message--people who embed videos in ages they are creating probably won't be using a debug client. therefore, they will not see this assert

@Hoikas

Hoikas Nov 1, 2014

Member

this should probably be a log message--people who embed videos in ages they are creating probably won't be using a debug client. therefore, they will not see this assert

This comment has been minimized.

@Mystler

Mystler Nov 1, 2014

Contributor

I assume the same goes for the audio track? Where would you recommend to log that message to?

EDIT: In my WIP, I replaced the hsAssert with a log entry to a movie.log file.

@Mystler

Mystler Nov 1, 2014

Contributor

I assume the same goes for the audio track? Where would you recommend to log that message to?

EDIT: In my WIP, I replaced the hsAssert with a log entry to a movie.log file.

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

That sounds good

@Hoikas

Hoikas Nov 1, 2014

Member

That sounds good

Show outdated Hide outdated Sources/Plasma/FeatureLib/pfMoviePlayer/plMoviePlayer.cpp
if (fPlate)
fPlate->SetVisible(false);
for (int i = 0; i < fCallbacks.size(); i++)

This comment has been minimized.

@Hoikas

Hoikas Nov 1, 2014

Member

range based for?

@Hoikas

Hoikas Nov 1, 2014

Member

range based for?

@Hoikas

This comment has been minimized.

Show comment
Hide comment
@Hoikas

Hoikas Nov 1, 2014

Member

I know I wrote the original code here, and it's probably awful that I'm going to say this, but I've since changed my mind about how our code formatting should look. It would be really awesome if we could get this to use the same style as DirtSand. My newer additions, such as pfPatcher sort of do this. The most relevant change here is that class and method definitions have a newline before the opening braces, but loops and conditional statements do not.

Also, I believe congratulations are in order for getting this to work!

Member

Hoikas commented Nov 1, 2014

I know I wrote the original code here, and it's probably awful that I'm going to say this, but I've since changed my mind about how our code formatting should look. It would be really awesome if we could get this to use the same style as DirtSand. My newer additions, such as pfPatcher sort of do this. The most relevant change here is that class and method definitions have a newline before the opening braces, but loops and conditional statements do not.

Also, I believe congratulations are in order for getting this to work!

@Mystler

This comment has been minimized.

Show comment
Hide comment
@Mystler

Mystler Nov 1, 2014

Contributor

Heh, I definitely agree. The Plasma code style is really annoying. In fact, when writing on this I sometimes had to fight with my VS editor, because it didn't like how I wrote. Neither did I.

If you say that it's time for a revolution, I'm going to jump right into the front lines.

Contributor

Mystler commented Nov 1, 2014

Heh, I definitely agree. The Plasma code style is really annoying. In fact, when writing on this I sometimes had to fight with my VS editor, because it didn't like how I wrote. Neither did I.

If you say that it's time for a revolution, I'm going to jump right into the front lines.

@Deledrius

This comment has been minimized.

Show comment
Hide comment
@Deledrius

Deledrius Nov 1, 2014

Member

It can be hard to decide how to write any given thing, since there's no consistent style through the entire codebase (which given its size and the length of time it was worked on, isn't surprising), or sometimes even in within the same project.

So, is it finally time to merge this: 8590ba4?

Member

Deledrius commented Nov 1, 2014

It can be hard to decide how to write any given thing, since there's no consistent style through the entire codebase (which given its size and the length of time it was worked on, isn't surprising), or sometimes even in within the same project.

So, is it finally time to merge this: 8590ba4?

@Mystler

This comment has been minimized.

Show comment
Hide comment
@Mystler

Mystler Nov 1, 2014

Contributor

Updated. One commit addresses what you pointed out, the other one changes the code style.

Contributor

Mystler commented Nov 1, 2014

Updated. One commit addresses what you pointed out, the other one changes the code style.

@Hoikas

This comment has been minimized.

Show comment
Hide comment
@Hoikas

Hoikas Nov 1, 2014

wait... why not simply auto?

wait... why not simply auto?

This comment has been minimized.

Show comment
Hide comment
@Mystler

Mystler Nov 2, 2014

Owner

Becaust const and because reference. ;)

Owner

Mystler replied Nov 2, 2014

Becaust const and because reference. ;)

@Hoikas

This comment has been minimized.

Show comment
Hide comment
@Hoikas

Hoikas Nov 1, 2014

same as 290

@Lunanne

This comment has been minimized.

Show comment
Hide comment
@Lunanne

Lunanne Nov 26, 2014

Member

Tested it and seems to work fine.
The only issue I found is that when you pres the escape key more than once to exit the intro video (because you're impatient and there is a slight delay) you can get the help gui to show up more than once. You can just click them away so I don't think this is a huge issue. The screen did flicker for me sometimes during the intro video, but that might be on my end.

Member

Lunanne commented Nov 26, 2014

Tested it and seems to work fine.
The only issue I found is that when you pres the escape key more than once to exit the intro video (because you're impatient and there is a slight delay) you can get the help gui to show up more than once. You can just click them away so I don't think this is a huge issue. The screen did flicker for me sometimes during the intro video, but that might be on my end.

@Deledrius

This comment has been minimized.

Show comment
Hide comment
@Deledrius

Deledrius Dec 23, 2014

Member

👍

Member

Deledrius commented Dec 23, 2014

👍

Hoikas added a commit that referenced this pull request Dec 26, 2014

Merge pull request #458 from Mystler/webm
WebM Movie Player (VP9/Opus)

@Hoikas Hoikas merged commit 990f42c into H-uru:master Dec 26, 2014

@Mystler Mystler deleted the Mystler:webm branch Dec 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment