Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798863005:
Use incoming frame's rotation while converting it to I420 (Closed)
|
Created:
3 years, 8 months ago by Chandan Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, emircan+watch+mediarecorder_chromium.org, posciak+watch_chromium.org, jam, darin-cc_chromium.org, mcasas+mediarecorder_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse incoming frame's rotation while converting it to I420
Rotation was not considered while converting incoming frame to I420.
This CL uses incoming frame's rotation for the conversion.
BUG=710068
TEST=H264 connection where recording the rotated remote stream on a
Mac results in a properly rotated stream.
Review-Url: https://codereview.chromium.org/2798863005
Cr-Commit-Position: refs/heads/master@{#463521}
Committed: https://chromium.googlesource.com/chromium/src/+/b06713810a8546bf4a5a135fe1fa2ef66d778e92
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed review comments #
Total comments: 11
Patch Set 3 : Addressed review comments #Messages
Total messages: 25 (12 generated)
c.padhi@samsung.com changed reviewers: + mcasas@chromium.org
PTAL.Thank you.
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ can you PTAL?
https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.cc:305: We should swap width and height of destination frame's size if rotation is 90 or 270. https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.cc:345: } Can we make this a switch statement? Additionally, it would be nice to move it in a static function MediaVideoRotationToRotationMode().
Thanks emircan@ for the review. I will make the necessary changes tomorrow as I am out of office currently. https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.cc:305: On 2017/04/06 17:01:22, emircan wrote: > We should swap width and height of destination frame's size if rotation is 90 or > 270. Swap width and height for all 3: video_frame->coded_size(), video_frame->visible_rect(), video_frame->natural_size()? https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.cc:345: } On 2017/04/06 17:01:22, emircan wrote: > Can we make this a switch statement? Additionally, it would be nice to move it > in a static function MediaVideoRotationToRotationMode(). Sure.
Please review. https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.cc:345: } On 2017/04/06 17:01:22, emircan wrote: > Can we make this a switch statement? Additionally, it would be nice to move it > in a static function MediaVideoRotationToRotationMode(). Done. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:332: visible_rect.set_height(visible_rect.width()); I am not too confident of this change. Please suggest.
https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:66: } Add "NOTREACHED();". It makes sure that we will be notified if it ever comes this section. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:323: gfx::Rect visible_rect = video_frame->visible_rect(); You can do all these calculations based on visible rectangle size. const gfx::Rect& old_visible_size = video_frame->visible_rect().size(); gfx::Rect new_visible_size = old_visible_size; ... new_visible_size.set_width(old_visible_size.height()); new_visible_size.set_height(old_visible_size.width()); ... frame = media::VideoFrame::CreateFrame(media::PIXEL_FORMAT_I420, new_visible_size, gfx::Rect(new_visible_size), new_visible_size, video_frame->timestamp()); https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:326: media::VideoFrameMetadata::ROTATION, &video_rotation)) { Merge two if statements with &&. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:332: visible_rect.set_height(visible_rect.width()); On 2017/04/07 13:21:55, Chandan wrote: > I am not too confident of this change. Please suggest. I can totally understand, sorry for my late response. Here is a summary of what those sizes stand for: - coded_size: The memory footprint of the frame. - visible_rect: Actual size of the frame that is a subset of coded_size. For instance, for a 571x265 frame, we might align memory to 572x266 for performance. Then 572x266 would be the coded_size and 0x0,571x265 would be visible_rect. - natural_size: visible_rect's size with aspect ratio adjustment. https://cs.chromium.org/chromium/src/media/base/video_frame.h?rcl=9c785e56c8f... In this particular case, we are only copying visible_rect's size portion of the texture into an SKImage, see l.340. So, that is what we are interested in. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:381: frame->visible_rect().width(), Use |old_visible_rect| here as well.
c.padhi@samsung.com changed reviewers: + a.suchit@chromium.org
Uploaded patch set 3. PTAL. Thank you. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:66: } On 2017/04/07 19:50:55, emircan wrote: > Add "NOTREACHED();". It makes sure that we will be notified if it ever comes > this section. Done. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:323: gfx::Rect visible_rect = video_frame->visible_rect(); On 2017/04/07 19:50:55, emircan wrote: > You can do all these calculations based on visible rectangle size. > > const gfx::Rect& old_visible_size = video_frame->visible_rect().size(); > gfx::Rect new_visible_size = old_visible_size; > ... > new_visible_size.set_width(old_visible_size.height()); > new_visible_size.set_height(old_visible_size.width()); > ... > frame = media::VideoFrame::CreateFrame(media::PIXEL_FORMAT_I420, > new_visible_size, gfx::Rect(new_visible_size), new_visible_size, > video_frame->timestamp()); Done. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:326: media::VideoFrameMetadata::ROTATION, &video_rotation)) { On 2017/04/07 19:50:55, emircan wrote: > Merge two if statements with &&. Done. https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:332: visible_rect.set_height(visible_rect.width()); On 2017/04/07 19:50:55, emircan wrote: > On 2017/04/07 13:21:55, Chandan wrote: > > I am not too confident of this change. Please suggest. > > I can totally understand, sorry for my late response. Here is a summary of what > those sizes stand for: > - coded_size: The memory footprint of the frame. > - visible_rect: Actual size of the frame that is a subset of coded_size. For > instance, for a 571x265 frame, we might align memory to 572x266 for performance. > Then 572x266 would be the coded_size and 0x0,571x265 would be visible_rect. > - natural_size: visible_rect's size with aspect ratio adjustment. > https://cs.chromium.org/chromium/src/media/base/video_frame.h?rcl=9c785e56c8f... > > In this particular case, we are only copying visible_rect's size portion of the > texture into an SKImage, see l.340. So, that is what we are interested in. Thanks! https://codereview.chromium.org/2798863005/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:381: frame->visible_rect().width(), On 2017/04/07 19:50:55, emircan wrote: > Use |old_visible_rect| here as well. Done.
Please open a Crbug and fill the BUG line for tracking this change. It helps tracking the changes and possible problems. Also, you should add a TEST line to help testers as it is not obvious how code would be covered, see https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list.... That being said, lgtm zand thanks for taking this :)
On 2017/04/10 18:00:50, emircan wrote: > Please open a Crbug and fill the BUG line for tracking this change. It helps > tracking the changes and possible problems. Also, you should add a TEST line to > help testers as it is not obvious how code would be covered, see > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list.... > > That being said, lgtm zand thanks for taking this :) emircan@, I ended up creating https://bugs.chromium.org/p/chromium/issues/detail?id=710068 via Wizard API, which doesn't look convincing. May be I do not have requisite permissions to edit Chromium Bug labels. If its possible on your part to create a new bug w.r.t this CL, I will add the same here else I shall use the one which I created. Thanks. TEST=content_unittests --gtest_filter="*VideoTrackRecorderTest.*" would suffice?
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/10 18:55:43, Chandan wrote: > On 2017/04/10 18:00:50, emircan wrote: > > Please open a Crbug and fill the BUG line for tracking this change. It helps > > tracking the changes and possible problems. Also, you should add a TEST line > to > > help testers as it is not obvious how code would be covered, see > > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list.... > > > > That being said, lgtm zand thanks for taking this :) > > emircan@, I ended up creating > https://bugs.chromium.org/p/chromium/issues/detail?id=710068 via Wizard API, > which doesn't look convincing. May be I do not have requisite permissions to > edit Chromium Bug labels. If its possible on your part to create a new bug w.r.t > this CL, I will add the same here else I shall use the one which I created. > Thanks. > > TEST=content_unittests --gtest_filter="*VideoTrackRecorderTest.*" would suffice? Sure. I edited the bug to point to the correct labels. For TEST line, you can refer to unittest as well, but i don't think VideoTrackRecorderTest covers this particular case of texture frame input. I looked for an example but there isn't a test that covers l.268 where we download texture bits. I will make a separate bug for it. Here you can mention how you manually tested as a reference to the testers, i.e. H264 connection where you are recording the rotated remote stream on a Mac results in properly rotated stream.
Description was changed from ========== Use incoming frame's rotation while converting it to I420 Rotation was not considered while converting incoming frame to I420. This CL uses incoming frame's rotation for the conversion. BUG=None ========== to ========== Use incoming frame's rotation while converting it to I420 Rotation was not considered while converting incoming frame to I420. This CL uses incoming frame's rotation for the conversion. BUG=710068 TEST=H264 connection where recording the rotated remote stream on a Mac results in a properly rotated stream. ==========
On 2017/04/10 19:24:55, emircan wrote: > On 2017/04/10 18:55:43, Chandan wrote: > > On 2017/04/10 18:00:50, emircan wrote: > > > Please open a Crbug and fill the BUG line for tracking this change. It helps > > > tracking the changes and possible problems. Also, you should add a TEST line > > to > > > help testers as it is not obvious how code would be covered, see > > > > > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list.... > > > > > > That being said, lgtm zand thanks for taking this :) > > > > emircan@, I ended up creating > > https://bugs.chromium.org/p/chromium/issues/detail?id=710068 via Wizard API, > > which doesn't look convincing. May be I do not have requisite permissions to > > edit Chromium Bug labels. If its possible on your part to create a new bug > w.r.t > > this CL, I will add the same here else I shall use the one which I created. > > Thanks. > > > > TEST=content_unittests --gtest_filter="*VideoTrackRecorderTest.*" would > suffice? > > Sure. I edited the bug to point to the correct labels. > > For TEST line, you can refer to unittest as well, but i don't think > VideoTrackRecorderTest covers this particular case of texture frame input. I > looked for an example but there isn't a test that covers l.268 where we download > texture bits. I will make a separate bug for it. Here you can mention how you > manually tested as a reference to the testers, i.e. H264 connection where you > are recording the rotated remote stream on a Mac results in properly rotated > stream. Thanks. Updated description with BUG and TEST lines.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by c.padhi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491882008211280,
"parent_rev": "e6866222bc8454a9fd7fe2209645484bb4be11fe", "commit_rev":
"f2fb2e4010f66b1d7538c72839899755993a941d"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491882008211280,
"parent_rev": "c57654688e304e8d48c8067d848ce3b048ae455b", "commit_rev":
"b06713810a8546bf4a5a135fe1fa2ef66d778e92"}
Message was sent while issue was closed.
Description was changed from ========== Use incoming frame's rotation while converting it to I420 Rotation was not considered while converting incoming frame to I420. This CL uses incoming frame's rotation for the conversion. BUG=710068 TEST=H264 connection where recording the rotated remote stream on a Mac results in a properly rotated stream. ========== to ========== Use incoming frame's rotation while converting it to I420 Rotation was not considered while converting incoming frame to I420. This CL uses incoming frame's rotation for the conversion. BUG=710068 TEST=H264 connection where recording the rotated remote stream on a Mac results in a properly rotated stream. Review-Url: https://codereview.chromium.org/2798863005 Cr-Commit-Position: refs/heads/master@{#463521} Committed: https://chromium.googlesource.com/chromium/src/+/b06713810a8546bf4a5a135fe1fa... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b06713810a8546bf4a5a135fe1fa... |
