Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10965033:
Set the config OMX_MirrorVertical to have same orientation as other video decoders
|
Created:
8 years, 3 months ago by shivdasp.nvidia Modified:
7 years, 1 month ago Reviewers:
Ami GONE FROM CHROMIUM CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCall SetConfig for OMX_IndexConfigCommonMirror (OMX_MirrorVertical) to have same Y-orientation as other video decoders. This is required only for NVIDIA OMX implementation.
BUG=chrome-os-partner:12403
TEST=Play any H264 video from USB and observe correct orientation
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:194: "OMX.Nvidia.h264ext.decode", true); indent is wrong https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:502: OMX_ERRORTYPE result = OMX_ErrorNone; unnecessary (and counter to chromium style; declare variables as late as possible). https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:523: // The OMX spec doesn't say whether (0,0) is bottom-left or top-left, but both indent is off https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:524: // OMX implementations used with this class far choose the same, and are the this isn't true anymore (which is why this code is inside the nvidia-specific block). https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:531: result = OMX_GetConfig( component_handle_, extra space https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:532: OMX_IndexConfigCommonMirror, &mirror_config); indent is off https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:535: result = OMX_SetConfig( component_handle_, extra space https://codereview.chromium.org/10965033/diff/1/content/common/gpu/media/omx_... content/common/gpu/media/omx_video_decode_accelerator.cc:536: OMX_IndexConfigCommonMirror, &mirror_config); indent is off
Incorporated review comments in patchset #2.
LGTM % nits https://codereview.chromium.org/10965033/diff/4001/content/common/gpu/media/o... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/10965033/diff/4001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:193: component_name_is_nvidia_h264ext_ = StartsWithASCII(component, either all args are indented to the opening ( or all are +4. Mixing styles like this (where |component| follows ( but "OMX... is +4) is not stylish. https://codereview.chromium.org/10965033/diff/4001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:523: // NVIDIA OMX implementation used with this class choose the opposite s/choose/chooses/
Incorporated review comments in patchset #3
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shivdasp.nvidia@gmail.com/10965033/2002
Presubmit check for 10965033-2002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** shivdasp.nvidia@gmail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA.
It looks like AUTHORS lists *@nvidia.com. Can you upload the final patch to a new rietveld issue from an nvidia.com address and point to it from here?
On 2012/09/21 18:19:36, I haz the power (commit-bot) wrote: > Presubmit check for 10965033-2002 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit Warnings ** > mailto:shivdasp.nvidia@gmail.com is not in AUTHORS file. If you are a new contributor, > please visit > http://www.chromium.org/developers/contributing-code and read the "Legal" > section > If you are a chromite, verify the contributor signed the CLA. Are you covered by a corporate CLA? Ami please verify. Even so, you should have your email address listed in the AUTHORS file. Also using an @nvidia.com email address would make more sense.
On 2012/09/21 18:21:15, Ami Fischman wrote: > It looks like AUTHORS lists mailto:*@nvidia.com. Can you upload the final patch > to a new rietveld issue from an http://nvidia.com address and point to it from > here? You have to upload a new CL, since Rietveld doesn't allow switching Author.
On 2012/09/21 18:21:42, Marc-Antoine Ruel wrote: > On 2012/09/21 18:21:15, Ami Fischman wrote: > > It looks like AUTHORS lists mailto:*@nvidia.com. Can you upload the final > patch > > to a new rietveld issue from an http://nvidia.com address and point to it from > > here? > > You have to upload a new CL, since Rietveld doesn't allow switching Author. I have uploaded another CL https://codereview.chromium.org/10968039/ but I guess the problem would persist. Strangely, while doing "gcl upload" , the "Email (login for uploading to https://codereview.chromium.org)" does show as shivdasp@nvidia.com Guess since I added Gmail to my GoogleID , the primary email has become shivdasp.nvidia@gmail.com instead of shivdasp@nvidia.com. Any suggestions how I could solve this ? I tried to change in Gmail settings but it doesn't allow.
Okay, I deleted my gmail account, the primary address is nvidia now. Let me now retry pushing a new CL. (Hopefully last one) Sorry for the noise.
Uploaded a new CL with correct AUTHOR. http://codereview.chromium.org/10967047/
Removing myself as reviewer to drop this from my CL list. |
