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

Optional GStreamer video playback support on MSW. #1947

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@PetrosKataras
Contributor

PetrosKataras commented Dec 28, 2017

This PR adds optional support for the GStreamer player under MSW 64bit through the CMake build system backend. It has only been tested on a VM running Windows 10 with Visual Studio 2015 so I m putting it here in case someone would like to give it a proper try.

You will need to have GStreamer installed and available on the environment in order for this to work. The latest version as of now is 1.12.4. You will need gstreamer-1.0-devel-x86_64-1.12.4.msi and gstreamer-1.0-x86_64-1.12.4.msi .

Choose the complete installation when prompt in order to get all necessary GStreamer plugins installed.

Once the installation is complete you should add the GStreamer installation bin and lib paths to your PATH env variable. By default these should be C:\gstreamer\1.0\x86_64\bin and C:\gstreamer\1.0\x86_64\lib

To enable it you will need to generate the VS files for Cinder with the following:

cmake .. -G "Visual Studio 14 2015 Win64" -DCINDER_MSW_USE_GSTREAMER=1

For testing you can build the QuickTimeBasic sample by generating VS project files through:

cmake .. -G "Visual Studio 14 2015 Win64"

@richardeakin richardeakin added the msw label Dec 28, 2017

source_group( "cinder\\app\\msw" FILES ${SRC_SET_APP_MSW} )
# Option for using GStreamer under MSW.
if( CINDER_MSW )
option( CINDER_MSW_USE_GSTREAMER "Use GStreamer for video playback." OFF )

This comment has been minimized.

@richardeakin

richardeakin Dec 30, 2017

Collaborator

I was thinking this would be a nice global option, for all platforms even. By default it can be on for linux, off for mac and windows. Not sure how we'd handle who gets priority for the different video backends, though (but then, qtime on windows is on it's last leg anyhow).

@richardeakin

richardeakin Dec 30, 2017

Collaborator

I was thinking this would be a nice global option, for all platforms even. By default it can be on for linux, off for mac and windows. Not sure how we'd handle who gets priority for the different video backends, though (but then, qtime on windows is on it's last leg anyhow).

This comment has been minimized.

@PetrosKataras

PetrosKataras Jan 2, 2018

Contributor

Yes, that would make sense probably. Lets see how everything pans out after a bit of testing and if people are happy with everything we can make this a global CINDER_USE_GSTREAMER option. In terms of priority I guess we can keep GStreamer default for Linux, second option for macOS and on Windows we would still have to see what makes more sense I guess..

@PetrosKataras

PetrosKataras Jan 2, 2018

Contributor

Yes, that would make sense probably. Lets see how everything pans out after a bit of testing and if people are happy with everything we can make this a global CINDER_USE_GSTREAMER option. In terms of priority I guess we can keep GStreamer default for Linux, second option for macOS and on Windows we would still have to see what makes more sense I guess..

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 3, 2018

Contributor

Just pushed two PR's that are relevant if someone wants to do some testing and should be applied before trying this out. Namely #1951 and #1952 . The first one fixes streaming sources and the second one makes the test app compatible with MSW ( depends on #1951 in order to work properly though ).

Contributor

PetrosKataras commented Jan 3, 2018

Just pushed two PR's that are relevant if someone wants to do some testing and should be applied before trying this out. Namely #1951 and #1952 . The first one fixes streaming sources and the second one makes the test app compatible with MSW ( depends on #1951 in order to work properly though ).

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Jan 3, 2018

Collaborator

Thanks so much for this Petros! Many people have been looking forward to giving gstreamer a shot on Windows for a while now so I'm sure many of us appreciate the contribution here.

I merged #1951 into master because it seemed like generally an important fix. How about combining #1952 with this PR and we can test / review all the MSW stuff together here?

Collaborator

richardeakin commented Jan 3, 2018

Thanks so much for this Petros! Many people have been looking forward to giving gstreamer a shot on Windows for a while now so I'm sure many of us appreciate the contribution here.

I merged #1951 into master because it seemed like generally an important fix. How about combining #1952 with this PR and we can test / review all the MSW stuff together here?

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 3, 2018

Contributor

Hey Rich, I closed #1952 and applied it here as you suggested. Probably faster for people interested in testing this indeed.

Contributor

PetrosKataras commented Jan 3, 2018

Hey Rich, I closed #1952 and applied it here as you suggested. Probably faster for people interested in testing this indeed.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Jan 6, 2018

Collaborator

So I fired this up this this evening and for starters, it built great right out of the box following the instructions above. I've only tried the QuicktimeBasic sample so far, and unfortunately it seems to hang after I've selected a video on this line. Here is the relevant stack trace:

QuickTimeBasic.exe!gst::video::GstPlayer::setPipelineState(GstState targetState=GST_STATE_PLAYING) Line 1003	C++
QuickTimeBasic.exe!gst::video::GstPlayer::play() Line 684	C++
QuickTimeBasic.exe!cinder::linux::MovieBase::play(bool toggle=false) Line 224	C++
QuickTimeBasic.exe!QuickTimeSampleApp::loadMovieFile(const std::experimental::filesystem::v1::path & moviePath={...}) Line 65	C++
QuickTimeBasic.exe!QuickTimeSampleApp::setup() Line 44	C++

I've tried loading .mp4 (created with the Windows 10 window+G game record functionality), .mov (created with an iphone), and some .vob files I have for testing surround sound, the main thread stops on that line with each. I can paste the other relevant thread info if that helps too (I see another background thread is waiting on this line).

I can try other video files if you think it may be a codec issue, or send some some of the files I've tried as well.

Side note: The g_print() calls in cinder code - are we able to see the output of these? I don't know if gstreamer routes them to the MS console already, but I was thinking it would be sweet if we could route those all through ci::log somehow, so they show up in all the logging backends and with metadata.

Collaborator

richardeakin commented Jan 6, 2018

So I fired this up this this evening and for starters, it built great right out of the box following the instructions above. I've only tried the QuicktimeBasic sample so far, and unfortunately it seems to hang after I've selected a video on this line. Here is the relevant stack trace:

QuickTimeBasic.exe!gst::video::GstPlayer::setPipelineState(GstState targetState=GST_STATE_PLAYING) Line 1003	C++
QuickTimeBasic.exe!gst::video::GstPlayer::play() Line 684	C++
QuickTimeBasic.exe!cinder::linux::MovieBase::play(bool toggle=false) Line 224	C++
QuickTimeBasic.exe!QuickTimeSampleApp::loadMovieFile(const std::experimental::filesystem::v1::path & moviePath={...}) Line 65	C++
QuickTimeBasic.exe!QuickTimeSampleApp::setup() Line 44	C++

I've tried loading .mp4 (created with the Windows 10 window+G game record functionality), .mov (created with an iphone), and some .vob files I have for testing surround sound, the main thread stops on that line with each. I can paste the other relevant thread info if that helps too (I see another background thread is waiting on this line).

I can try other video files if you think it may be a codec issue, or send some some of the files I've tried as well.

Side note: The g_print() calls in cinder code - are we able to see the output of these? I don't know if gstreamer routes them to the MS console already, but I was thinking it would be sweet if we could route those all through ci::log somehow, so they show up in all the logging backends and with metadata.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 6, 2018

Contributor

The second link indicates that there has been an error so it would be great to see the output. Unfortunately it seems that g_print will only work with a Cygwin64 terminal so if you could run the QuickTimeBasic from there we should get more info about the root of the issue. On my VM this translates to opening a Cygwin64 terminal, changing directory to where the QuickTimeBasic.exe lives and running ./QuickTimeBasic.exe.

This is how it looks on my VM:

screen shot 2018-01-06 at 9 45 38 am

To me this seems like an installation issue where some plugins are missing. Just to double check; You chose the COMPLETE and not the TYPICAL installation option when installing both GStreamer binaries right ?

On my installation I had success running the sample from inside Cygwin64, from inside a Command Prompt and directly from inside Visual Studio but the first option only offers console output currently.

Another way you can verify that your installation is complete, is by using gst-launch-1.0. This is a standard GStreamer command line utility that is part of the installation packages.

From inside Command Prompt and assuming a user a3nao and that a test.avi file is located under the Downloads folder you can run :
gst-launch-1.0 playbin uri=file:///C:/Users/a3nao/Downloads/test.avi

This is how the output looks on my system:

screen shot 2018-01-06 at 10 23 19 am

gst-launch-1.0 properly prints on the console so this option should also give us an understanding of what is happening here. If the above works then it should also work with the GstPlayer otherwise we have to dig deeper...

Obligatory screenshot of the QuickTimeBasic sample running inside Visual Studio which I forgot so far to attach...

screen shot 2018-01-06 at 8 57 57 am

Contributor

PetrosKataras commented Jan 6, 2018

The second link indicates that there has been an error so it would be great to see the output. Unfortunately it seems that g_print will only work with a Cygwin64 terminal so if you could run the QuickTimeBasic from there we should get more info about the root of the issue. On my VM this translates to opening a Cygwin64 terminal, changing directory to where the QuickTimeBasic.exe lives and running ./QuickTimeBasic.exe.

This is how it looks on my VM:

screen shot 2018-01-06 at 9 45 38 am

To me this seems like an installation issue where some plugins are missing. Just to double check; You chose the COMPLETE and not the TYPICAL installation option when installing both GStreamer binaries right ?

On my installation I had success running the sample from inside Cygwin64, from inside a Command Prompt and directly from inside Visual Studio but the first option only offers console output currently.

Another way you can verify that your installation is complete, is by using gst-launch-1.0. This is a standard GStreamer command line utility that is part of the installation packages.

From inside Command Prompt and assuming a user a3nao and that a test.avi file is located under the Downloads folder you can run :
gst-launch-1.0 playbin uri=file:///C:/Users/a3nao/Downloads/test.avi

This is how the output looks on my system:

screen shot 2018-01-06 at 10 23 19 am

gst-launch-1.0 properly prints on the console so this option should also give us an understanding of what is happening here. If the above works then it should also work with the GstPlayer otherwise we have to dig deeper...

Obligatory screenshot of the QuickTimeBasic sample running inside Visual Studio which I forgot so far to attach...

screen shot 2018-01-06 at 8 57 57 am

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 6, 2018

Contributor

Also thinking a bit more about it I remember having some issues, on macOS at least, that seemed to be caused from the installation order of the GStreamer packages. For the shake of it I would make sure that gstreamer-1.0-x86_64-1.12.4.msi is installed before gstreamer-1.0-devel-x86_64-1.12.4.msi

Contributor

PetrosKataras commented Jan 6, 2018

Also thinking a bit more about it I remember having some issues, on macOS at least, that seemed to be caused from the installation order of the GStreamer packages. For the shake of it I would make sure that gstreamer-1.0-x86_64-1.12.4.msi is installed before gstreamer-1.0-devel-x86_64-1.12.4.msi

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 7, 2018

Contributor

#1954 replaces all g_print* occurrences with the relevant CI_LOG_* alternatives. With this, messages are being routed to MS console so it should make debugging easier on that front ( i.e no need anymore to run from a Cygwin64 terminal to get debug output as described on my previous message. )

Contributor

PetrosKataras commented Jan 7, 2018

#1954 replaces all g_print* occurrences with the relevant CI_LOG_* alternatives. With this, messages are being routed to MS console so it should make debugging easier on that front ( i.e no need anymore to run from a Cygwin64 terminal to get debug output as described on my previous message. )

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Jan 7, 2018

Collaborator

Yep did a complete install, though, I may have installed the devel package first, I can't recall exactly. I'll try to remove the packages and retry in that order.

If we merge #1954, will all the messages in your screenshot show up in Visual Studio console? I didn't realize you could even run an .exe in cygwin (and don't have cygwin installed, either. Most use the linux bash shell nowadays for something like that).

I'll report back after I can get everything reinstalled.

Collaborator

richardeakin commented Jan 7, 2018

Yep did a complete install, though, I may have installed the devel package first, I can't recall exactly. I'll try to remove the packages and retry in that order.

If we merge #1954, will all the messages in your screenshot show up in Visual Studio console? I didn't realize you could even run an .exe in cygwin (and don't have cygwin installed, either. Most use the linux bash shell nowadays for something like that).

I'll report back after I can get everything reinstalled.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 8, 2018

Contributor

Yes, with #1954 all messages appear in Visual Studio console also. Give it a try and hopefully we can see where things go wrong.

Contributor

PetrosKataras commented Jan 8, 2018

Yes, with #1954 all messages appear in Visual Studio console also. Give it a try and hopefully we can see where things go wrong.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Jan 10, 2018

Contributor

Just got my hands on a proper Windows machine today and I had the chance to try this out. It seems that running on a VM made context sharing between Cinder and GStreamer less picky but the last commit should fix it when running native. There is still a context sharing issue on MSW if one enables the async loading option so I have opened a ticket in GStreamer upstream to try and get more info about the root but this should be a good start I think.

Contributor

PetrosKataras commented Jan 10, 2018

Just got my hands on a proper Windows machine today and I had the chance to try this out. It seems that running on a VM made context sharing between Cinder and GStreamer less picky but the last commit should fix it when running native. There is still a context sharing issue on MSW if one enables the async loading option so I have opened a ticket in GStreamer upstream to try and get more info about the root but this should be a good start I think.

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 12, 2018

Hi, it seems I'm missing something:
cmake .. -G "Visual Studio 15 2017 Win64" -DCINDER_MSW_USE_GSTREAMER=1
-- The C compiler identification is MSVC 19.13.26128.0
-- The CXX compiler identification is MSVC 19.13.26128.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
CMake Warning:
Manually-specified variables were not used by the project:
CINDER_MSW_USE_GSTREAMER
-- Build files have been written to: .../Cinder/proj

I think I have to modify the files concerned with this PR.
Why not enabling this CINDER_USE_GSTREAMER option by default on Windows?

brucelane commented Mar 12, 2018

Hi, it seems I'm missing something:
cmake .. -G "Visual Studio 15 2017 Win64" -DCINDER_MSW_USE_GSTREAMER=1
-- The C compiler identification is MSVC 19.13.26128.0
-- The CXX compiler identification is MSVC 19.13.26128.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/VC/Tools/MSVC/14.13.26128/bin/Hostx86/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
CMake Warning:
Manually-specified variables were not used by the project:
CINDER_MSW_USE_GSTREAMER
-- Build files have been written to: .../Cinder/proj

I think I have to modify the files concerned with this PR.
Why not enabling this CINDER_USE_GSTREAMER option by default on Windows?

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Mar 12, 2018

Contributor

Not sure why you would have to modify anything to get this going -- Just to be sure, you have applied locally this PR on top of current master and you are seeing this output?

CINDER_MSW_USE_GSTREAMER is checked here so I don't understand why you would get an unused variable warning.

This would need more testing before considering it as the default option for Windows I believe..

Contributor

PetrosKataras commented Mar 12, 2018

Not sure why you would have to modify anything to get this going -- Just to be sure, you have applied locally this PR on top of current master and you are seeing this output?

CINDER_MSW_USE_GSTREAMER is checked here so I don't understand why you would get an unused variable warning.

This would need more testing before considering it as the default option for Windows I believe..

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 12, 2018

sorry Petros, I haven't apply this PR, that's why

brucelane commented Mar 12, 2018

sorry Petros, I haven't apply this PR, that's why

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Mar 12, 2018

Collaborator

@PetrosKataras finally got around to trying this again, after uninstalling gstreamer packages and re-installing them in the order you suggested. Everything I seem to throw at it now is working great! Including sound and fullscreen.

It feels like this could warrant some documentation after we've merged this PR, perhaps we should open a separate ticket for this? I think we could start with a wiki page here on this cinder repo, though eventually we'll probably want something similar (and to eventually replace) the QuickTime Movie Write Guide.

To point out the obvious, it is clear that eventually we'll want to refactor things so that we aren't using a ci::linux::Movie on windows (and macosx too). :) But I suppose this has to do with how the next video api will look.

Excellent work and thank you again for this! I think we finally have a video implementation that is working across all our supported platforms (well maybe not iOS, but AVFoundation works fine there). I think it seems appropriate to get this into master branch in the current form so that it can see wider user testing before we plan out the next ci::video steps.

Collaborator

richardeakin commented Mar 12, 2018

@PetrosKataras finally got around to trying this again, after uninstalling gstreamer packages and re-installing them in the order you suggested. Everything I seem to throw at it now is working great! Including sound and fullscreen.

It feels like this could warrant some documentation after we've merged this PR, perhaps we should open a separate ticket for this? I think we could start with a wiki page here on this cinder repo, though eventually we'll probably want something similar (and to eventually replace) the QuickTime Movie Write Guide.

To point out the obvious, it is clear that eventually we'll want to refactor things so that we aren't using a ci::linux::Movie on windows (and macosx too). :) But I suppose this has to do with how the next video api will look.

Excellent work and thank you again for this! I think we finally have a video implementation that is working across all our supported platforms (well maybe not iOS, but AVFoundation works fine there). I think it seems appropriate to get this into master branch in the current form so that it can see wider user testing before we plan out the next ci::video steps.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Mar 12, 2018

Collaborator

Also considering being used as a default on windows: this is sort of difficult because it requires installing the additional gstreamer packages, and also quite a slew of compiler flags to each project. So I think it is a good option on windows, but not exactly sure if it can be a default, either.

Collaborator

richardeakin commented Mar 12, 2018

Also considering being used as a default on windows: this is sort of difficult because it requires installing the additional gstreamer packages, and also quite a slew of compiler flags to each project. So I think it is a good option on windows, but not exactly sure if it can be a default, either.

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 12, 2018

I tryed today to refactor this and make a sample called GStreamerBasic, but along the way I found too many dependencies on qtime namespace, quicktime includes, so I stopped... once was said on the forums we should move away from quicktime, I think gstreamer is the way to go, I can handle gstreamer setup, maybe others wouldn't like it...

brucelane commented Mar 12, 2018

I tryed today to refactor this and make a sample called GStreamerBasic, but along the way I found too many dependencies on qtime namespace, quicktime includes, so I stopped... once was said on the forums we should move away from quicktime, I think gstreamer is the way to go, I can handle gstreamer setup, maybe others wouldn't like it...

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Mar 12, 2018

Collaborator

@brucelane what are you trying to refactor? The way things currently are, you can sort of ignore that qtime really means quicktime - this happened organically (during the original linux port) as gstreamer just became a backend for qtime::MovieGl on platforms that support it. In the future, the plan is that it'll all get refactored into a more generic namespace (something like ci::video but it isn't decided on yet).

Collaborator

richardeakin commented Mar 12, 2018

@brucelane what are you trying to refactor? The way things currently are, you can sort of ignore that qtime really means quicktime - this happened organically (during the original linux port) as gstreamer just became a backend for qtime::MovieGl on platforms that support it. In the future, the plan is that it'll all get refactored into a more generic namespace (something like ci::video but it isn't decided on yet).

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 12, 2018

I was trying to make a sample without any reference to quicktime. ci::video makes sense!

brucelane commented Mar 12, 2018

I was trying to make a sample without any reference to quicktime. ci::video makes sense!

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 14, 2018

ok, I made a sample https://github.com/videodromm/VideodrommVideoPlayer
I added Cinder-Warping also...

brucelane commented Mar 14, 2018

ok, I made a sample https://github.com/videodromm/VideodrommVideoPlayer
I added Cinder-Warping also...

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 14, 2018

gst
thank you for this!

brucelane commented Mar 14, 2018

gst
thank you for this!

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Mar 14, 2018

Collaborator

I think we should go ahead and merge this, only thing I can think of first: should we cut down on the logging? I think ideally the video player wouldn't produce any logs at all unless something goes wrong. What I usually do is place the logging behind a macro that so I can easily re-enable, like here for example.

Not sure why the travis build doesn't show completed, but it is.

Collaborator

richardeakin commented Mar 14, 2018

I think we should go ahead and merge this, only thing I can think of first: should we cut down on the logging? I think ideally the video player wouldn't produce any logs at all unless something goes wrong. What I usually do is place the logging behind a macro that so I can easily re-enable, like here for example.

Not sure why the travis build doesn't show completed, but it is.

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 20, 2018

please merge !

brucelane commented Mar 20, 2018

please merge !

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Mar 20, 2018

Collaborator

I agree we should merge. @PetrosKataras are all the ShellStreamFolder messages something internal to gstreamer? I'm referring to these messages:

ShellStreams: AttachShellStreams: InitializedTrying to load localized resources for LANGID 1033 with Primary LANGID 9 code en name English (Universal)Resource DLL loaded at path C:\Program Files\Common Files\Apple\Internet Services\ShellStreams.resources\en.lproj\ShellStreamsLocalized.dllNot running under explorer. Will not set thread locale18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A0810 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A0810 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
...

I'm also seeing a few ReturnHr logs, but every cinder app is creating at least a couple of these in more recent sdk versions. Not sure what we can do about those, but I'm just wondering if there is a way to reduce the verbosity. Can merge anyway to keep things rolling.

Collaborator

richardeakin commented Mar 20, 2018

I agree we should merge. @PetrosKataras are all the ShellStreamFolder messages something internal to gstreamer? I'm referring to these messages:

ShellStreams: AttachShellStreams: InitializedTrying to load localized resources for LANGID 1033 with Primary LANGID 9 code en name English (Universal)Resource DLL loaded at path C:\Program Files\Common Files\Apple\Internet Services\ShellStreams.resources\en.lproj\ShellStreamsLocalized.dllNot running under explorer. Will not set thread locale18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A0810 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A0810 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
pidl = 1f50e04fd020ea3a6910a2d8802b30309d140 -> 2e80853fd6f0ec379740b3d61b4a891711800 -> 
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::CreateViewObject
18448 00000229A63A12B0 ENTER: ShellStreamsFolder::ShellStreamsFolder
...

I'm also seeing a few ReturnHr logs, but every cinder app is creating at least a couple of these in more recent sdk versions. Not sure what we can do about those, but I'm just wondering if there is a way to reduce the verbosity. Can merge anyway to keep things rolling.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Mar 20, 2018

Contributor

Hey, sorry for being late in the discussion but it has been very busy lately. Happy to hear that it works for more people. As mentioned, the sync loading should work but I would be interested to know if the same applies when enabling the async loading option. You can try this by enabling this option and recompiling.

This didn't work on my test hardware which was running Windows 10 with a GeForce GTX 1080 ( don't remember driver version and I m away from that machine currently ). I have also filed a bug upstream with GStreamer here but unfortunately I haven't found the time to investigate further so far why context sharing between Cinder and GStreamer fails in that case.

Btw, with the player you can also force the use of the traditional CPU path which is slower but it will pretty much work on any hardware combo. You can do that by forcing this option to false. This should probably be turned into an actual compile time option and all these different scenarios and options should be properly documented. I hope I can find some time to gather some of these in the wiki sooner than later.

@richardeakin I don't remember seeing these messages before although I might be mistaken but I doubt they come from GStreamer since it follows quite a different format in general. Are you using Visual Studio 2015 to compile the binaries?

Contributor

PetrosKataras commented Mar 20, 2018

Hey, sorry for being late in the discussion but it has been very busy lately. Happy to hear that it works for more people. As mentioned, the sync loading should work but I would be interested to know if the same applies when enabling the async loading option. You can try this by enabling this option and recompiling.

This didn't work on my test hardware which was running Windows 10 with a GeForce GTX 1080 ( don't remember driver version and I m away from that machine currently ). I have also filed a bug upstream with GStreamer here but unfortunately I haven't found the time to investigate further so far why context sharing between Cinder and GStreamer fails in that case.

Btw, with the player you can also force the use of the traditional CPU path which is slower but it will pretty much work on any hardware combo. You can do that by forcing this option to false. This should probably be turned into an actual compile time option and all these different scenarios and options should be properly documented. I hope I can find some time to gather some of these in the wiki sooner than later.

@richardeakin I don't remember seeing these messages before although I might be mistaken but I doubt they come from GStreamer since it follows quite a different format in general. Are you using Visual Studio 2015 to compile the binaries?

@brucelane

This comment has been minimized.

Show comment
Hide comment
@brucelane

brucelane Mar 23, 2018

Please let me know if I can help for the ci::video code/test

brucelane commented Mar 23, 2018

Please let me know if I can help for the ci::video code/test

@gordeaoux

This comment has been minimized.

Show comment
Hide comment
@gordeaoux

gordeaoux Mar 24, 2018

@PetrosKataras For the async loading option, it looks like it fails on wglShareLists in gstreamer's gl context with the error 0xaa, which I think is ERROR_BUSY . My best guess there is that since it's getting starting asynchronously, it's also trying to get the context at a time when it's in use elsewhere in cinder. There might be a way to share the context immediately with the pipeline without waiting for the sync message on the bus, but I wasn't able to get that to work.

Here's the error with failing contexts in async:
gstglbasefilter.c gst_gl_base_filter_decide_allocation error: failed to share contexts through wglShareLists 0xaa

Also, this works with 1.14.0.1 as well, with the following changes:

Line 16:

#if defined( CINDER_GST_HAS_GL )
#include <gst/gl/gl.h>
#endif

Line 156 (GST_GL_TYPE_CONTEXT is now GST_TYPE_GL_CONTEXT):

#ifdef GST_TYPE_GL_CONTEXT
			gst_structure_set(s, "context", GST_TYPE_GL_CONTEXT, data.context, nullptr);
#else 
			gst_structure_set(s, "context", GST_GL_TYPE_CONTEXT, data.context, nullptr);
#endif

gordeaoux commented Mar 24, 2018

@PetrosKataras For the async loading option, it looks like it fails on wglShareLists in gstreamer's gl context with the error 0xaa, which I think is ERROR_BUSY . My best guess there is that since it's getting starting asynchronously, it's also trying to get the context at a time when it's in use elsewhere in cinder. There might be a way to share the context immediately with the pipeline without waiting for the sync message on the bus, but I wasn't able to get that to work.

Here's the error with failing contexts in async:
gstglbasefilter.c gst_gl_base_filter_decide_allocation error: failed to share contexts through wglShareLists 0xaa

Also, this works with 1.14.0.1 as well, with the following changes:

Line 16:

#if defined( CINDER_GST_HAS_GL )
#include <gst/gl/gl.h>
#endif

Line 156 (GST_GL_TYPE_CONTEXT is now GST_TYPE_GL_CONTEXT):

#ifdef GST_TYPE_GL_CONTEXT
			gst_structure_set(s, "context", GST_TYPE_GL_CONTEXT, data.context, nullptr);
#else 
			gst_structure_set(s, "context", GST_GL_TYPE_CONTEXT, data.context, nullptr);
#endif

gordeaoux added a commit to Downstream/ds_cinder that referenced this pull request Mar 24, 2018

GstVideo: Attempt to integrate GstGL elements (WIP)
- Implemented GstPlayer from cinder/Cinder#1947
- Uses a shared OpenGL context to upload textures from another thread
- Somewhat better performance for very large videos (4k+). Biggest bonus is videos don't block the main thread when uploading the texture or copying bytes. Still suffers from video decode CPU bottleneck.
- Would need an abstracted API from gst_video and the option to switch out backed
- Needs clean up and a bunch of stuff implemented to be functional
@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Mar 24, 2018

Contributor

@gordeaoux

Hi and thanks for testing. Not sure if you checked the link I provided on my previous comment regarding the bug report that I have opened upstream with GStreamer but it basically describes how the situation currently has and some tips on what should ideally happen. Unfortunately I haven't found the time to follow up so far.

Very cool that you got the API updates for the latest version of GStreamer, was something I also wanted to do in the near future.

Contributor

PetrosKataras commented Mar 24, 2018

@gordeaoux

Hi and thanks for testing. Not sure if you checked the link I provided on my previous comment regarding the bug report that I have opened upstream with GStreamer but it basically describes how the situation currently has and some tips on what should ideally happen. Unfortunately I haven't found the time to follow up so far.

Very cool that you got the API updates for the latest version of GStreamer, was something I also wanted to do in the near future.

@gordeaoux

This comment has been minimized.

Show comment
Hide comment
@gordeaoux

gordeaoux Mar 24, 2018

Ah yeah, figured more logs could be helpful. I see the thread now and you already knew about all that: https://bugzilla.gnome.org/show_bug.cgi?id=792407

I got async working with Matthew's suggestion of overriding the create-context signal: https://gist.github.com/gordeaoux/cf9f7f08d549a106c1ac51ccefaf535c

The downside is that it hangs for a second or so when starting the first video while the context is created (similar to the sync-mode hang), but subsequent video plays are faster.

gordeaoux commented Mar 24, 2018

Ah yeah, figured more logs could be helpful. I see the thread now and you already knew about all that: https://bugzilla.gnome.org/show_bug.cgi?id=792407

I got async working with Matthew's suggestion of overriding the create-context signal: https://gist.github.com/gordeaoux/cf9f7f08d549a106c1ac51ccefaf535c

The downside is that it hangs for a second or so when starting the first video while the context is created (similar to the sync-mode hang), but subsequent video plays are faster.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Mar 26, 2018

Contributor

Thanks for following up with this - To be honest I don't see why it would hang for a second or so during context sharing. To my opinion this should be a relatively fast operation so I m wondering if there is something else happening here. The sync-hang actually comes from waiting for the pipeline to pre-roll before continuing further. Unfortunately I m away from a native Windows machine until next week and on the VM with the mesa drivers this operation succeeds ( and pretty fast actually ) so I cannot really test currently from my side.

Contributor

PetrosKataras commented Mar 26, 2018

Thanks for following up with this - To be honest I don't see why it would hang for a second or so during context sharing. To my opinion this should be a relatively fast operation so I m wondering if there is something else happening here. The sync-hang actually comes from waiting for the pipeline to pre-roll before continuing further. Unfortunately I m away from a native Windows machine until next week and on the VM with the mesa drivers this operation succeeds ( and pretty fast actually ) so I cannot really test currently from my side.

@gordeaoux

This comment has been minimized.

Show comment
Hide comment
@gordeaoux

gordeaoux Mar 26, 2018

In async mode, I added a call to gst_gl_context_create that Visual Studio reported taking 1,884 ms to complete. In sync mode, the first time a video is loaded there's a similar 1.5-2.0 second delay in the main thread (though I wasn't able to time since I don't have the gstreamer debug symbols). So I suspect the delay in both cases is gstreamer's creation of the context. This could also be a product of my specific system (native Win10, NVidia k5000 with driver 390.77) and perhaps other GL systems start faster.

The above is only for the first video load, subsequent loads are much quicker. And as you noted, the sync version does hang for the pre-roll.

Since the context could be created before the video, it might be possible to make the initialize() function of GstPlayer static so the gstreamer gl context could be created on app startup, rather than when a video loads. I'll respond in the gstreamer bug, as perhaps there's a way to get the context created quicker.

gordeaoux commented Mar 26, 2018

In async mode, I added a call to gst_gl_context_create that Visual Studio reported taking 1,884 ms to complete. In sync mode, the first time a video is loaded there's a similar 1.5-2.0 second delay in the main thread (though I wasn't able to time since I don't have the gstreamer debug symbols). So I suspect the delay in both cases is gstreamer's creation of the context. This could also be a product of my specific system (native Win10, NVidia k5000 with driver 390.77) and perhaps other GL systems start faster.

The above is only for the first video load, subsequent loads are much quicker. And as you noted, the sync version does hang for the pre-roll.

Since the context could be created before the video, it might be possible to make the initialize() function of GstPlayer static so the gstreamer gl context could be created on app startup, rather than when a video loads. I'll respond in the gstreamer bug, as perhaps there's a way to get the context created quicker.

@num3ric

This comment has been minimized.

Show comment
Hide comment
@num3ric

num3ric Apr 20, 2018

Contributor

So a CI_CHECK_GL() right after the MovieGl::create() call reveals a GL_INVALID_OPERATION I have yet to find the cause for. Is this related to the issues @gordeaoux mentioned?

Contributor

num3ric commented Apr 20, 2018

So a CI_CHECK_GL() right after the MovieGl::create() call reveals a GL_INVALID_OPERATION I have yet to find the cause for. Is this related to the issues @gordeaoux mentioned?

@num3ric

This comment has been minimized.

Show comment
Hide comment
@num3ric

num3ric Apr 21, 2018

Contributor

So yes, investigating this further, the gl invalid operation is related to the disabling and re-enabling attempt of the gl context, similar to what a user reported here it seems: https://www.opengl.org/discussion_boards/showthread.php/165903-wglMakeCurrent

If call gl::makeCurrent( true ) (with the force boolean), the error moves away from glGetError to a gstreamer one. Still not sure how to solve this. (This is in occurring with VS2017, x64.)

Contributor

num3ric commented Apr 21, 2018

So yes, investigating this further, the gl invalid operation is related to the disabling and re-enabling attempt of the gl context, similar to what a user reported here it seems: https://www.opengl.org/discussion_boards/showthread.php/165903-wglMakeCurrent

If call gl::makeCurrent( true ) (with the force boolean), the error moves away from glGetError to a gstreamer one. Still not sure how to solve this. (This is in occurring with VS2017, x64.)

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Apr 22, 2018

Contributor

@num3ric Could you try the workaround that @gordeaoux implemented ? ( i.e overriding the create-context signal )

To achieve this you would need the following in your GstPlayer.cpp file.

After this line add:

static GstGLContext* sGstAsyncContext = nullptr;
static GstGLContext* create_gl_context( GstGLDisplay* display, GstGLContext* context, gpointer userData ) {
    return sGstAsyncContext;
}

Inside the checkBusMessageSync callback change this line to:

if( sGstAsyncContext ) {
    gst_structure_set( s, "context", GST_GL_TYPE_CONTEXT, sGstAsyncContext, nullptr );
} else {
    gst_structure_set( s, "context", GST_GL_TYPE_CONTEXT, data.context, nullptr );
}

Then inside the initialize function add the following after this line:

if( ! sGstAsyncContext ) {
    sGstAsyncContext = gst_gl_context_new( sGstGLDisplay );
    GError* err = nullptr;
    if( ! gst_gl_context_create(sGstAsyncContext, mGstData.context, &err ) && err ) {
        CI_LOG_E( "Failed to create ctx with error : "  << err->message );
    }
} else {
    gst_object_ref( sGstAsyncContext );
}
if( sGstAsyncContext ) {
    g_signal_connect( sGstGLDisplay, "create-context", G_CALLBACK( create_gl_context ), nullptr, nullptr );
}

And finally cleanup by adding the following after this line:

if( sGstAsyncContext ) {
    gst_object_unref(sGstAsyncContext);
}

Note that this might still require a call to gl::makeCurrent( true ) after the player creation.

Contributor

PetrosKataras commented Apr 22, 2018

@num3ric Could you try the workaround that @gordeaoux implemented ? ( i.e overriding the create-context signal )

To achieve this you would need the following in your GstPlayer.cpp file.

After this line add:

static GstGLContext* sGstAsyncContext = nullptr;
static GstGLContext* create_gl_context( GstGLDisplay* display, GstGLContext* context, gpointer userData ) {
    return sGstAsyncContext;
}

Inside the checkBusMessageSync callback change this line to:

if( sGstAsyncContext ) {
    gst_structure_set( s, "context", GST_GL_TYPE_CONTEXT, sGstAsyncContext, nullptr );
} else {
    gst_structure_set( s, "context", GST_GL_TYPE_CONTEXT, data.context, nullptr );
}

Then inside the initialize function add the following after this line:

if( ! sGstAsyncContext ) {
    sGstAsyncContext = gst_gl_context_new( sGstGLDisplay );
    GError* err = nullptr;
    if( ! gst_gl_context_create(sGstAsyncContext, mGstData.context, &err ) && err ) {
        CI_LOG_E( "Failed to create ctx with error : "  << err->message );
    }
} else {
    gst_object_ref( sGstAsyncContext );
}
if( sGstAsyncContext ) {
    g_signal_connect( sGstGLDisplay, "create-context", G_CALLBACK( create_gl_context ), nullptr, nullptr );
}

And finally cleanup by adding the following after this line:

if( sGstAsyncContext ) {
    gst_object_unref(sGstAsyncContext);
}

Note that this might still require a call to gl::makeCurrent( true ) after the player creation.

@num3ric

This comment has been minimized.

Show comment
Hide comment
@num3ric

num3ric Apr 24, 2018

Contributor

@PetrosKataras Thanks for the line-by-line here, it works. GL_INVALID_OPERATION is no more. (With gl::makeCurrent( true ))

Contributor

num3ric commented Apr 24, 2018

@PetrosKataras Thanks for the line-by-line here, it works. GL_INVALID_OPERATION is no more. (With gl::makeCurrent( true ))

@num3ric

This comment has been minimized.

Show comment
Hide comment
@num3ric

num3ric Apr 25, 2018

Contributor

Quick reminder to add CI_API to the MovieBase and MovieGl classes to support shared library builds!

Contributor

num3ric commented Apr 25, 2018

Quick reminder to add CI_API to the MovieBase and MovieGl classes to support shared library builds!

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Apr 27, 2018

Contributor

I believe the last commit should fix the issue for both sync and async loading without the need to manually force the context back on the application level ( i.e without a call to ci::gl::context()->makeCurrent( true ) )

If someone could confirm that it would be great and we could then finalize this.

Contributor

PetrosKataras commented Apr 27, 2018

I believe the last commit should fix the issue for both sync and async loading without the need to manually force the context back on the application level ( i.e without a call to ci::gl::context()->makeCurrent( true ) )

If someone could confirm that it would be great and we could then finalize this.

@PetrosKataras

This comment has been minimized.

Show comment
Hide comment
@PetrosKataras

PetrosKataras Apr 30, 2018

Contributor

Not sure if rebasing was the best option but trying to manually merge or cherry-picking #2003 didn't seem to resolve the conflict.

Sorry for the noise - Managed to apply it manually in the end so that we don't get all the history re-write that rebase brings along.

Contributor

PetrosKataras commented Apr 30, 2018

Not sure if rebasing was the best option but trying to manually merge or cherry-picking #2003 didn't seem to resolve the conflict.

Sorry for the noise - Managed to apply it manually in the end so that we don't get all the history re-write that rebase brings along.

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