Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384823002:
Add SetVideoMode API to avsettings (Closed)
|
Created:
4 years, 2 months ago by almasrymina Modified:
4 years, 2 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gfhuang+watch_chromium.org, halliwell+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SetVideoMode API to avsettings
BUG= internal b/31777725
TEST= None
Committed: https://crrev.com/99b70c41d5d9d40e1b5cac04f8ccb7edefbcc6d9
Cr-Commit-Position: refs/heads/master@{#423555}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Addressed comments. #
Total comments: 4
Patch Set 4 : Addressed comments #Messages
Total messages: 24 (9 generated)
Description was changed from ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None Change-Id: I161fdd1a38de986f3be42d632fa8100658e04318 ========== to ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None Change-Id: I161fdd1a38de986f3be42d632fa8100658e04318 ==========
almasrymina@chromium.org changed reviewers: + halliwell@chromium.org, jasonroberts@google.com, wzhong@chromium.org
Please review API.
gfhuang@chromium.org changed reviewers: + gfhuang@chromium.org
https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsettings.h File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:300: } public API must be pure. and from the comment, it's not clear to me how does this apply to CastTV and AndroidTV.
Will change to pure. If a platform wants to support dynamic mode switching, for power consumption reasons or to optimize viewing experience (if it makes sense), then they should implement this. Otherwise return false.
On 2016/09/30 20:50:08, almasrymina wrote: > Will change to pure. > > If a platform wants to support dynamic mode switching, for power consumption > reasons or to optimize viewing experience (if it makes sense), then they should > implement this. Otherwise return false. I believe the prefer_50hz and prefer_fps is HDMI only. I am not sure about what allow_4k means. If all 3 options are HDMI only, just rename the API to be SetHdmiVideoMode. and add a comment saying only applies to devices that outputs HDMI.
https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsettings.h File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:300: } On 2016/09/30 20:30:24, gfhuang wrote: > public API must be pure. > > and from the comment, it's not clear to me how does this apply to CastTV and > AndroidTV. I actually wonder if we should group the HDMI-related functions in a separate HdmiSettings interface (AvSettings could have GetHdmi function to access it, and Cast TV could just return null, then they can ignore all HDMI_related changes). Thoughts?
https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsettings.h File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:284: // prefer_50hz: picks a resolution optimized for 50 hz content. can we eliminate this in public API (use the fps parameter instead). I know we might be stuck with that internally but the public API is harder to change. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:286: // optimize_for_fps: *Attempts* to pick an fps optimal for the given fps. "pick a refresh rate optimal for the given content frame rate" https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:292: // Resolution set is already the best resolution with the params provided. 'resolution is already best' should not be the same return value as error case. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:294: // Returns True otherwise. A SCREEN_INFO_CHANGED event fires when the video nit: 'true' https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:298: int optimize_for_fps) { can we make this a float (or express as fps*100) so we can distinguish e.g. 29.97 from 30fps
https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsettings.h File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:284: // prefer_50hz: picks a resolution optimized for 50 hz content. On 2016/09/30 21:28:14, halliwell wrote: > can we eliminate this in public API (use the fps parameter instead). I know we > might be stuck with that internally but the public API is harder to change. Done. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:286: // optimize_for_fps: *Attempts* to pick an fps optimal for the given fps. On 2016/09/30 21:28:14, halliwell wrote: > "pick a refresh rate optimal for the given content frame rate" Done. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:292: // Resolution set is already the best resolution with the params provided. On 2016/09/30 21:28:14, halliwell wrote: > 'resolution is already best' should not be the same return value as error case. Discussed offline: the error cases here are not things the app care or can do anything about, so the design is around this: if the API returns true, wait for a SCREEN_INFO_CHANGED event before proceeding. If the API returns false, go ahead and start playback without waiting. I could change this so it returns false in the HDMI disconnected case, and true in the 'Resolution already set' case, and fire the event always, but that doesn't seem nice to me. I don't imagine the app cares if HDMI is connected or not, the app will probably want to start playback anyway since the user is asking for that. ANd if the app cares about whether we're connected or not, we do fire HDMI_CONNECTED and DISCONNECTED events and they should use that and not the return value here. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:294: // Returns True otherwise. A SCREEN_INFO_CHANGED event fires when the video On 2016/09/30 21:28:14, halliwell wrote: > nit: 'true' Done. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:298: int optimize_for_fps) { On 2016/09/30 21:28:14, halliwell wrote: > can we make this a float (or express as fps*100) so we can distinguish e.g. > 29.97 from 30fps Done. https://codereview.chromium.org/2384823002/diff/1/chromecast/public/avsetting... chromecast/public/avsettings.h:300: } On 2016/09/30 20:30:24, gfhuang wrote: > public API must be pure. > > and from the comment, it's not clear to me how does this apply to CastTV and > AndroidTV. Done.
Looking close. Couple of nits on the comment. Also it's not great having argument called 'fps' that's actually fps*100, I wonder if that name could be improved. https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:311: // mode setting completes. When this API returns false, no event is fired. Could we just tighten up/organise this whole comment a bit? * Use |allow_4k| notation for arguments * 23.98hz example is wrong, should say 2398 * Return value description is very disorganised, could be more succinct and keep the whole description of true/false together. e.g. something like this: Returns: - true if mode change is beginning. Caller should wait for SCREEN_INFO_CHANGED event for mode change to complete. - false if no made change has begun. This could be because HDMI is disconnected, or the current resolution is already good for the given parameters.
https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:295: // Sets the video mode according to the given parameters: HDMI video mode https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:313: // This API only applies to devices that output HDMI. ", otherwise, simply return false" (or true?)
Description was changed from ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None Change-Id: I161fdd1a38de986f3be42d632fa8100658e04318 ========== to ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None ==========
almasrymina@google.com changed reviewers: + almasrymina@google.com
https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:295: // Sets the video mode according to the given parameters: On 2016/10/03 20:33:59, gfhuang wrote: > HDMI video mode Done. https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:311: // mode setting completes. When this API returns false, no event is fired. On 2016/10/03 20:27:34, halliwell wrote: > Could we just tighten up/organise this whole comment a bit? > * Use |allow_4k| notation for arguments > * 23.98hz example is wrong, should say 2398 > * Return value description is very disorganised, could be more succinct and keep > the whole description of true/false together. e.g. something like this: > > Returns: > - true if mode change is beginning. Caller should wait for SCREEN_INFO_CHANGED > event for mode change to complete. > - false if no made change has begun. This could be because HDMI is > disconnected, or the current resolution is already good for the given > parameters. Done. I couldn't think of a better name, so I just made it very clear in the comment. Suggestions welcome though :) https://codereview.chromium.org/2384823002/diff/20001/chromecast/public/avset... chromecast/public/avsettings.h:313: // This API only applies to devices that output HDMI. On 2016/10/03 20:33:59, gfhuang wrote: > ", otherwise, simply return false" (or true?) Done.
lgtm % couple more comment nits. https://codereview.chromium.org/2384823002/diff/40001/chromecast/public/avset... File chromecast/public/avsettings.h (right): https://codereview.chromium.org/2384823002/diff/40001/chromecast/public/avset... chromecast/public/avsettings.h:296: // nit, can lose blank line https://codereview.chromium.org/2384823002/diff/40001/chromecast/public/avset... chromecast/public/avsettings.h:298: // ditto https://codereview.chromium.org/2384823002/diff/40001/chromecast/public/avset... chromecast/public/avsettings.h:309: // nit, lose one of these blank lines. https://codereview.chromium.org/2384823002/diff/40001/chromecast/public/avset... chromecast/public/avsettings.h:311: // This API only applies to devices that output HDMI, otherwise return false. This sentence is weird. Just "Non-HDMI devices should return false." ?
The CQ bit was checked by almasrymina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/2384823002/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None ========== to ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None ========== to ========== Add SetVideoMode API to avsettings BUG= internal b/31777725 TEST= None Committed: https://crrev.com/99b70c41d5d9d40e1b5cac04f8ccb7edefbcc6d9 Cr-Commit-Position: refs/heads/master@{#423555} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/99b70c41d5d9d40e1b5cac04f8ccb7edefbcc6d9 Cr-Commit-Position: refs/heads/master@{#423555} |
