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

Refactor 3D settings, improve playback handling and integrate into skins #4

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
3 participants
@da-anda
Copy link

da-anda commented May 26, 2013

This PR adds a central class called StereoscopicsManager which holds generic stereoscopic related functions to be used by video players and other classes. It also contains the most of the logic for stereoscopic related user interactions (changing settings, dialogs, ...) and performs the necessary tasks (switching GUI into 3D mode).

It also adds a central toggle to enable/disable stereoscopic features as well as several playback related options:

  • define a preferred playback mode for stereoscopic movies (ask, preferred stereoscopic mode, ignore/don't to anything automatically)
  • specify a preferred stereoscopic mode in which according movies should be rendered/converted to
  • turn off stereoscopic mode if video playback stopped

The last commit is my attempt to integrate those features nicely into the video OSD, so that they can be controlled/changed via remotes.

edit: Note that this won't compile on OSX/iOS yet because I didn't know how to add the StereoscopicsManager.cpp to the xcode projects. Feel free to create a PR on my branch with according changes and I'll add them to this PR.

@da-anda

This comment has been minimized.

Copy link
Author

da-anda commented May 26, 2013

@baijuxavior you might be interested in this :)

@baijuxavior

This comment has been minimized.

Copy link

baijuxavior commented May 27, 2013

Great work. Gonna try them.

@elupus

This comment has been minimized.

Copy link
Owner

elupus commented May 27, 2013

I'm pretty sure all setting strings should be lowercase.

@elupus

View changes

xbmc/guilib/GraphicContext.cpp Outdated
if (g_Windowing.SupportsStereo(RENDER_STEREO_MODE_INTERLACED))
supportedModes.insert(RENDER_STEREO_MODE_INTERLACED, RENDER_STEREO_MODE_INTERLACED);
if (g_Windowing.SupportsStereo(RENDER_STEREO_MODE_HARDWAREBASED))
supportedModes.insert(RENDER_STEREO_MODE_HARDWAREBASED, RENDER_STEREO_MODE_HARDWAREBASED);

This comment has been minimized.

Copy link
@elupus

elupus May 27, 2013

Owner

Can be simplified by a for loop from STEREO_MODE_OFF to STEREO_MODE_COUNT (if that doesn't exit in the enum, add it last)

This comment has been minimized.

Copy link
@elupus

elupus May 27, 2013

Owner

But with above change, you don't even need the function.

@elupus

View changes

xbmc/guilib/GraphicContext.cpp Outdated
else
it++;

SetStereoMode((RENDER_STEREO_MODE) it->second);

This comment has been minimized.

Copy link
@elupus

elupus May 27, 2013

Owner
mode = original_mode;
do {
  mode = (mode + 1) % STEREO_MODE_COUNT;
  if(g_windowing.support(mode) {
    SetStereoMode()
    break;
  } while(mode != original_mode);
@da-anda

This comment has been minimized.

Copy link
Author

da-anda commented May 29, 2013

@elupus, I fixed all compile and functional issues that where still present in this RFC draft locally already, but before I waste time on a interactive rebase again I wanted to hear your opinion on a thought. I somehow would now prefer to move most of the logic (settings dependencies handling, settings fillers, notification handling, aggregating of supported 3D modes, cAction handling, file name parsing for 3D type detection,...) to some sort of StereoscopicsManager class, so that f.e. the GraphicContext doesn't have to know anything about settings and their relation etc. If this is fine with you, I'd need some advice where to put such a class and how to call it. Stereoscopics[Manager / Handler / Utils / Service]?

edit: is it somehow possible for the GUI/skin to know in which 3D format the currently played video is? If not, can this info be exposed?

@elupus

This comment has been minimized.

Copy link
Owner

elupus commented May 29, 2013

If that reduces complexity in the files I'm all for it. Most settings will
interact with gfx or app, so the risk is you still need to add accessories
functions if you try to put too much there.

Static utility functions would make sense to factor out.

Also we likely should rename the setting strings to be stereo something
instead of 3d.

@da-anda

This comment has been minimized.

Copy link
Author

da-anda commented May 29, 2013

yes, was thinking about accessory functions - at least where it makes sense. Will also rename settings to "stereomode" if you prefer that. So you're in general fine with moving stuff to a dedicated stereoscopics class. Any suggestion where to put it? xbmc/guilib/StereoscopicsManager.cpp ?

@elupus

This comment has been minimized.

Copy link
Owner

elupus commented May 29, 2013

Yea seem reasonable.

@elupus

View changes

xbmc/guilib/GraphicContext.cpp Outdated
mode = (mode + 1) % RENDER_STEREO_MODE_COUNT;
if(g_Windowing.SupportsStereo(mode))
break;
} while(mode != originalMode);

This comment has been minimized.

Copy link
@elupus

elupus Jun 1, 2013

Owner

i would factor this out to a function like:

enum mode = next_mode(enum current, int step)
{
  enum mode;
  do {
     mode = (mode + step) % RENDER_STEREO_MODE_COUNT;
     if(g_Windowing.SupportsStereo(mode))
        break;
   } while(mode != current);
   return mode;
}

The just call as:

SetStereoMode(next_mode(current, 1));

or

SetStereoMode(next_mode(current, RENDER_STEREO_MODE_COUNT - 1));

This comment has been minimized.

Copy link
@da-anda

da-anda Jun 2, 2013

Author

did so in my 3d-refactor branch

@da-anda

This comment has been minimized.

Copy link
Author

da-anda commented Jun 5, 2013

completely refactored this PR now. When I splitted my code into separate commits I might have added one or two .h #includes a bit too early (wrong commit), but as the splitting already took me ~3 hours I'm really not in the mood to fix it now.

@elupus elupus closed this Jun 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.