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

[json]: - added Player.SetViewMode/GetViewmode method to the json interface #15477

Merged
merged 1 commit into from Feb 22, 2019

Conversation

@Memphiz
Copy link
Member

Memphiz commented Feb 9, 2019

this bumps json api version to 10.2.0. For this I need help on how to handle the json api bump.
@MartijnKaijser as reviewer because I hope he knows who else I can ask :D

Description

expose the RenderManager.SetViewMode/GetViewMode method to json

Motivation and Context

This was requested in the forum and is useful for projector users. When having a projector with 16:9 screen and watching 21:9 movies it is good to shift the whole videos own to the bottom of the screen to get a hard cut in contrast (black bars are usually not that black when projected). By exposing ViewMode to json it is now possible to set vertical shift to the liking.
I use the same approach for my projector too.
On Wetek I used the amlogic hardware feature to change the render rect. But now that I have switched to intel hardware I have to achieve the same thing via json now.

How Has This Been Tested?

Tested via curl:

shift to the bottom ( vertical shift 1.0 means shift down as far as possible without cut):
curl -s --data-binary '{"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": {"verticalshift":1.0}},"id":1}' -H 'content-type: application/json;' http://localhost:8080/jsonrpc

back to normal view mode
curl -s --data-binary '{"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": "normal"},"id":1}' -H 'content-type: application/json;' http://localhost:8080/jsonrpc

query current viewmode
curl -s --data-binary '{"jsonrpc":"2.0","method":"Player.GetViewMode","id":1}' -H 'content-type: application/json;' http://localhost:8080/jsonrpc

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@Memphiz Memphiz requested a review from MartijnKaijser Feb 9, 2019

Show resolved Hide resolved xbmc/interfaces/json-rpc/PlayerOperations.cpp Outdated
Show resolved Hide resolved xbmc/interfaces/json-rpc/PlayerOperations.cpp Outdated
Show resolved Hide resolved xbmc/interfaces/json-rpc/PlayerOperations.cpp Outdated
Show resolved Hide resolved xbmc/interfaces/json-rpc/PlayerOperations.cpp Outdated
Show resolved Hide resolved xbmc/interfaces/json-rpc/PlayerOperations.cpp Outdated

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from 48d40ce to 0f4d941 Feb 10, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 10, 2019

@Rechi adapted

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Feb 10, 2019

@DaveTBlake can you do sanity check on json stuff?

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 10, 2019

Especially if the wiki is generated from the Schema or do I need to add the new method manually - and if the version bump is correct for an added method.

@rmrector

This comment has been minimized.

Copy link
Contributor

rmrector commented Feb 10, 2019

The wiki is not generated from schema, and it's quite out of date already. FWIW I always prefer browsing the live API with something like Chorus2's The Lab.

The version bump is good.

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from 0f4d941 to eb9d05e Feb 10, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 10, 2019

I will add the new method to the wiki once this PR is merged disregarding that it is outdated already :) ...

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 10, 2019

The Lab - nice to know ... thats a huge tool :)

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from eb9d05e to 46f018e Feb 10, 2019

@pkerling pkerling added this to the Leia 18.1-rc1 milestone Feb 10, 2019

@pkerling pkerling removed the 18.1 label Feb 10, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 11, 2019

@Memphiz thanks for doing some JSON.
The version bump is correct (adding a method which is a non-breaking change).

I don't think there is a wiki page for v10 JSON schema yet. One was started for v9 by mistake (the odd versions are interim during dev phase) and some things edited but most not. I think that that a partially updated and inaccurate page is worse than none at all, so unless someone volunteers to write a complete and accurate wiki for v10 I suggest it better to not have one. The schema is self documenting (description method) so it is not as if there is nothing for clients to look at.

But I do have some questions about the method

  • If there is a method to set viewmode then should there not also be a method to read the current viewmode?
  • Are the "viewmode" integers codified somewhere?
  • Do the double parameters have min and max values? E.g. negative zoom, so will any value be OK?
  • How can just one parameter be set and not the others? The JSON default could be different from the system default or current active value. Perhaps allow null values, and when a parameter is null use the current active value.
@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 11, 2019

The viewmode has an enum

enum ViewMode

But I struggled to duplicate it here because it would need adaptions to Json whenever the enum changed. It’s almost guaranteed to divert.

I can look for min max values for the double values though and add them to the Schema ...

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 11, 2019

To add - if viewmode is a value which is not in the enum it will simply fallback to „normal“ viewmode. The inner code guarantees that.

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from 46f018e to ba6de20 Feb 11, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 11, 2019

@DaveTBlake I tried my best. Have added GetViewMode in addition and used reference types which are nullable. Whenever a parameter is not given the former setting is reused.

@Memphiz Memphiz changed the title [json]: - added Player.ViewMode method to the json interface [json]: - added Player.SetViewMode/GetViewmode method to the json interface Feb 11, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 13, 2019

Disregard ... browser Cache played tricks on me and showed I need to rebase again which is not true luckily

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 14, 2019

Thanks for this @Memphiz definitely better, and sorry for delay getting back to you (been busy in real life). But I think it could be improved further. Those using the API should not have to look at C++ to know what the viewmode codes mean, the API should fully document and validate all parameters. Yes this means keeping the JSON schema upto date with any enum changes in core code, but that is how API consumers know something has changed.

Also looking deeper only some parameter combinations are valid. Calls to SetRenderViewMode either set viewmode to one of the mode presets e.g. Normal, Zoom, Stretch4x3 etc. or set view mode to Custom and set one or more of zoom, pixelratio, verticalshift or nonlinearstretch. This can be implemented in JSON too.

Define Player.SetViewMode

"Player.SetViewMode": {
    "type": "method",
    "description": "Set view mode of video player",
    "transport": "Response",
    "permission": "ControlPlayback",
    "params": [
      { "name": "viewmode", "type": [
          { "$ref": "Player.CustomViewMode", "description": "Custom view mode", "required": true },
          { "name": "value", "$ref": "Player.ViewMode", "required": true}
        ],
      "required": true }
    ],
    "returns": "string"
  },

using new types added to https://github.com/xbmc/xbmc/blob/master/xbmc/interfaces/json-rpc/schema/types.json with

  "Player.ViewMode": {
    "type": "string",
    "enum": [  "normal", "zoom", "stretch4x3", "widezoom", "stretch16x9", "original",
               "stretch16x9nonlin", "zoom120width", "zoom110width" ]
  },
  "Player.CustomViewMode": {
    "type": "object",
    "required": true,
    "properties": {
      "zoom": { "$ref": "Optional.Number", "minimum":0.5, "maximum": 2.0, "description": "Zoom were 1.0 means 100%" },
      "pixelratio": { "$ref": "Optional.Number", "minimum":0.5, "maximum": 2.0, "description": "Pixel aspect ratio were 1.0 means square pixel" },
      "verticalshift": { "$ref": "Optional.Number", "minimum": -2.0, "maximum": 2.0, "description": "Verticalshift 1.0 means shift to bottom" },
      "nonlinearstretch": { "$ref": "Optional.Boolean", "description": "Flag to enable nonlinear stretch" }
    }
  },

Example valid calls then look like

Normal mode
{"id":1,"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": "normal"}}
Zoom 110 width
{"id":1,"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": "zoom110width"}}
Custom with zoom and vertical shift
{"id":1,"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": {"zoom": 1.6, "verticalshift": 0.5}}}
Custom with pixel ratio
{"id":1,"jsonrpc":"2.0","method":"Player.SetViewMode","params":{"viewmode": {"pixelratio": 1.01}}}

I pulled the max/min values for zoom, pixelratio and verticalshift from https://github.com/xbmc/xbmc/blob/master/xbmc/video/PlayerController.cpp, it makes sense to me they should be the same (assuming they exist for a reason?). I can let you have a patch file for this if that helps.

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 14, 2019

So is our JSON API in that shape you suggest here everywhere else? This will take me a while to do it. I considered this an advanced feature but I guess everything in that API needs to be bullet proof.
Tbh - I start regretting doing this PR at all. My time is very limited atm. Looks like there are no quick win PRs common anymore nowadays.
Putting this on hold for now.

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 15, 2019

Yeap, needs to be bullet proof even if the internal function it exposes does not have the cleanest interface. Understand the time limits etc. @Memphiz, and sorry for any frustration. This can still be a quickish win. I have a patch file that implements what I'm suggesting, shall I push it to your repo (if I can)? Work all done, just a matter of getting into into this PR :)

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 15, 2019

Sure or to my repo and i merge it into this PR - thx

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 16, 2019

@Memphiz I have raised a PR for this against your repo, not sure if that is best way so if you need me to do something else then just let me know.

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from ba6de20 to 03dc81b Feb 17, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 17, 2019

@DaveTBlake i have added your commit to this PR and added my refactoring proposal. Main goal was to reduce the code duplication, early returns and if nesting. Also we have the string to enum mapping in a single place now which makes it easier to add new values later on. Once you are ok with this (and maybe you even have some time to test it) I will squash this into a single commit.

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from 03dc81b to 23e1354 Feb 17, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 17, 2019

@Memphiz I'm fine in principle with the refactoring, I obviously don't have as much aversion to early returns as you do and tend to copy existing code style :)

Only thought looking at the code is that IsValidViewmodeParameter() isn't needed as the JSON schema definition ensures the parameters are valid in that way. A request with something other than "increase"/"decrease" or a double would be rejected before SetViewMode is called. I can see how you would derive it from my code, it was my mistake too.

@DaveTBlake DaveTBlake added the v18 Leia label Feb 17, 2019

@Memphiz Memphiz force-pushed the Memphiz:viewmode_json branch from 23e1354 to 5241f76 Feb 17, 2019

@Memphiz

This comment has been minimized.

Copy link
Member Author

Memphiz commented Feb 17, 2019

updated and squashed @DaveTBlake

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 17, 2019

Great @Memphiz, thanks for this.
I have updated the test call example in the first post, and I will do some more thorough testing of all the variations later but looks good.

Any idea what is up with jenkins build?

@DaveTBlake
Copy link
Member

DaveTBlake left a comment

Done extensive testing and all looking good.
This is ready to merge as far as I'm concerned, and a useful JSON improvement for 18.2

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 18, 2019

Lets get a green tick first
jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Feb 18, 2019

jenkins build this please

@DaveTBlake DaveTBlake merged commit 90c7746 into xbmc:master Feb 22, 2019

1 check passed

default You're awesome. Have a cookie
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment