Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1352243002:
Implemented Multiple video track recoding.
|
Created:
5 years, 3 months ago by msu.koo Modified:
5 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented Multiple video track recoding.
Previosly, Only single video track recoding was supported.
This patch enhanced WebmMuxer to handle multiple video tracks
to support Multiple Video Track Recoding.
webm_muxer_unittest also updated to check this patch.
unittested with MediaRecorderHandlerTest, WebmMuxerTest.
BUG=528523
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : Comment addressed #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 20 (3 generated)
msu.koo@samsung.com changed reviewers: + ajose@chromium.org, mcasas@chromium.org
WebmMuxer is enhanced to handle multiple video tracks as mcasas commented. Please kindly review :)
Nice! https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... media/capture/webm_muxer.h:59: struct MuxerArg { Change to VideoMuxerArg or similar? These tests will eventually need to test audio too, so nice to have clear names in this regard. https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; muxer_args_ (or muxer_args_for_video_ or something)
Thank you for your review!. All your comments are reflected. :) https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... media/capture/webm_muxer.h:59: struct MuxerArg { On 2015/09/18 18:28:58, ajose wrote: > Change to VideoMuxerArg or similar? These tests will eventually need to test > audio too, so nice to have clear names in this regard. Done. https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; On 2015/09/18 18:28:58, ajose wrote: > muxer_args_ (or muxer_args_for_video_ or something) Done.
On 2015/09/21 04:28:37, msu.koo wrote: > Thank you for your review!. > All your comments are reflected. :) > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h > File media/capture/webm_muxer.h (right): > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > media/capture/webm_muxer.h:59: struct MuxerArg { > On 2015/09/18 18:28:58, ajose wrote: > > Change to VideoMuxerArg or similar? These tests will eventually need to test > > audio too, so nice to have clear names in this regard. > > Done. > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; > On 2015/09/18 18:28:58, ajose wrote: > > muxer_args_ (or muxer_args_for_video_ or something) > > Done. Hey, sorry to keep you waiting, there's: https://codereview.chromium.org/1354863002/ changing a few minor things in this area, I'll get to review this CL shortly, if it's OK with you.
On 2015/09/22 18:43:29, mcasas wrote: > On 2015/09/21 04:28:37, msu.koo wrote: > > Thank you for your review!. > > All your comments are reflected. :) > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h > > File media/capture/webm_muxer.h (right): > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > media/capture/webm_muxer.h:59: struct MuxerArg { > > On 2015/09/18 18:28:58, ajose wrote: > > > Change to VideoMuxerArg or similar? These tests will eventually need to test > > > audio too, so nice to have clear names in this regard. > > > > Done. > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; > > On 2015/09/18 18:28:58, ajose wrote: > > > muxer_args_ (or muxer_args_for_video_ or something) > > > > Done. > > Hey, sorry to keep you waiting, there's: > https://codereview.chromium.org/1354863002/ > changing a few minor things in this area, I'll get > to review this CL shortly, if it's OK with you. Anytime you ready :)
On 2015/09/23 00:01:32, msu.koo wrote: > On 2015/09/22 18:43:29, mcasas wrote: > > On 2015/09/21 04:28:37, msu.koo wrote: > > > Thank you for your review!. > > > All your comments are reflected. :) > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h > > > File media/capture/webm_muxer.h (right): > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > media/capture/webm_muxer.h:59: struct MuxerArg { > > > On 2015/09/18 18:28:58, ajose wrote: > > > > Change to VideoMuxerArg or similar? These tests will eventually need to > test > > > > audio too, so nice to have clear names in this regard. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; > > > On 2015/09/18 18:28:58, ajose wrote: > > > > muxer_args_ (or muxer_args_for_video_ or something) > > > > > > Done. > > > > Hey, sorry to keep you waiting, there's: > > https://codereview.chromium.org/1354863002/ > > changing a few minor things in this area, I'll get > > to review this CL shortly, if it's OK with you. > > Anytime you ready :) All right, the mentioned CL has landed, so please rebase and we can give it a spin. Mind const, grammar in comments and meaningful variable names (no abbreviations). Cheers!
On 2015/09/23 01:13:41, mcasas wrote: > On 2015/09/23 00:01:32, msu.koo wrote: > > On 2015/09/22 18:43:29, mcasas wrote: > > > On 2015/09/21 04:28:37, msu.koo wrote: > > > > Thank you for your review!. > > > > All your comments are reflected. :) > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h > > > > File media/capture/webm_muxer.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > > media/capture/webm_muxer.h:59: struct MuxerArg { > > > > On 2015/09/18 18:28:58, ajose wrote: > > > > > Change to VideoMuxerArg or similar? These tests will eventually need to > > test > > > > > audio too, so nice to have clear names in this regard. > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > > media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; > > > > On 2015/09/18 18:28:58, ajose wrote: > > > > > muxer_args_ (or muxer_args_for_video_ or something) > > > > > > > > Done. > > > > > > Hey, sorry to keep you waiting, there's: > > > https://codereview.chromium.org/1354863002/ > > > changing a few minor things in this area, I'll get > > > to review this CL shortly, if it's OK with you. > > > > Anytime you ready :) > > All right, the mentioned CL has landed, so please rebase > and we can give it a spin. Mind const, grammar in comments > and meaningful variable names (no abbreviations). Cheers! Sorry for late response. I was OOO. Patch is rebased. PTAL. :)
On 2015/09/30 03:53:43, msu.koo wrote: > On 2015/09/23 01:13:41, mcasas wrote: > > On 2015/09/23 00:01:32, msu.koo wrote: > > > On 2015/09/22 18:43:29, mcasas wrote: > > > > On 2015/09/21 04:28:37, msu.koo wrote: > > > > > Thank you for your review!. > > > > > All your comments are reflected. :) > > > > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h > > > > > File media/capture/webm_muxer.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > > > media/capture/webm_muxer.h:59: struct MuxerArg { > > > > > On 2015/09/18 18:28:58, ajose wrote: > > > > > > Change to VideoMuxerArg or similar? These tests will eventually need > to > > > test > > > > > > audio too, so nice to have clear names in this regard. > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1352243002/diff/1/media/capture/webm_muxer.h#... > > > > > media/capture/webm_muxer.h:86: std::vector<MuxerArg> muxer_args; > > > > > On 2015/09/18 18:28:58, ajose wrote: > > > > > > muxer_args_ (or muxer_args_for_video_ or something) > > > > > > > > > > Done. > > > > > > > > Hey, sorry to keep you waiting, there's: > > > > https://codereview.chromium.org/1354863002/ > > > > changing a few minor things in this area, I'll get > > > > to review this CL shortly, if it's OK with you. > > > > > > Anytime you ready :) > > > > All right, the mentioned CL has landed, so please rebase > > and we can give it a spin. Mind const, grammar in comments > > and meaningful variable names (no abbreviations). Cheers! > > Sorry for late response. I was OOO. > Patch is rebased. PTAL. :) lgtm
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352243002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
@mcasas, Could you take a look this CL?
https://chromiumcodereview.appspot.com/1352243002/diff/40001/content/renderer... File content/renderer/media/media_recorder_handler.cc (right): https://chromiumcodereview.appspot.com/1352243002/diff/40001/content/renderer... content/renderer/media/media_recorder_handler.cc:81: if (video_track.isNull()) { Is this possible at all? https://chromiumcodereview.appspot.com/1352243002/diff/40001/media/capture/we... File media/capture/webm_muxer.h (right): https://chromiumcodereview.appspot.com/1352243002/diff/40001/media/capture/we... media/capture/webm_muxer.h:53: int GetNextVideoTrackIndex(); Should be const. Actually it shouldn't exist at all. Extending to multiple track recording is a tad more complex than this, since we don't want to surface implementation details to clients. What I'd suggest is to add a method to WebmMuxer that generates OnEncodedVideoCallbacks with the parameter |track_index| bound in. The callers will then piggyback the remaining four parameters. (See [1] for a refresher). On a parallel note, adding a method for observing class X member is, well, a last option :) -- Instead, you would add an accessor to XTest and use it in the individual test cases, that are public subclasses of XTest. And, if there' no other option or would be too messy, use the suffix ForTesting so it'd be removed from release, non-test builds. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=120 https://chromiumcodereview.appspot.com/1352243002/diff/40001/media/capture/we... media/capture/webm_muxer.h:85: std::vector<VideoMuxerArg> video_muxer_args_; What about a std::map indexed by track indexes?
@mcasas, Than you for your review :) Your comments were addressed. PTAL https://codereview.chromium.org/1352243002/diff/40001/content/renderer/media/... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1352243002/diff/40001/content/renderer/media/... content/renderer/media/media_recorder_handler.cc:81: if (video_track.isNull()) { On 2015/10/06 17:53:13, mcasas wrote: > Is this possible at all? I cannot see the routine checking the validity of |video_track| before here. If you mind, how about DCHECK? https://codereview.chromium.org/1352243002/diff/40001/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1352243002/diff/40001/media/capture/webm_muxe... media/capture/webm_muxer.h:53: int GetNextVideoTrackIndex(); On 2015/10/06 17:53:13, mcasas wrote: > Should be const. > > Actually it shouldn't exist at all. Extending to multiple track recording > is a tad more complex than this, since we don't want to surface > implementation details to clients. What I'd suggest is to add a > method to WebmMuxer that generates OnEncodedVideoCallbacks > with the parameter |track_index| bound in. The callers will then > piggyback the remaining four parameters. (See [1] for a refresher). > > On a parallel note, adding a method for observing class X member is, > well, a last option :) -- Instead, you would add an accessor to XTest > and use it in the individual test cases, that are public subclasses of > XTest. And, if there' no other option or would be too messy, use the > suffix ForTesting so it'd be removed from release, non-test builds. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=120 Cool way! Thank you for your advice. reflected. https://codereview.chromium.org/1352243002/diff/40001/media/capture/webm_muxe... media/capture/webm_muxer.h:85: std::vector<VideoMuxerArg> video_muxer_args_; On 2015/10/06 17:53:13, mcasas wrote: > What about a std::map indexed by track indexes? I considered to use Map for this, but using vector can minimize the overhead then using map here because |track_index| will be assigned sequentially from muxer([1]). So I would leave as it is. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...
@mcasas, Could you take a look this CL?
A few comments. Have you tried using this on an HTML demo page? If so, please mention it in the TEST=line of the CL description. https://chromiumcodereview.appspot.com/1352243002/diff/60001/content/renderer... File content/renderer/media/media_recorder_handler.h (right): https://chromiumcodereview.appspot.com/1352243002/diff/60001/content/renderer... content/renderer/media/media_recorder_handler.h:65: // bool is_key_frame); ? https://chromiumcodereview.appspot.com/1352243002/diff/60001/media/capture/we... File media/capture/webm_muxer.h (right): https://chromiumcodereview.appspot.com/1352243002/diff/60001/media/capture/we... media/capture/webm_muxer.h:93: std::vector<VideoMuxerArg> video_muxer_args_; what about an std::map indexed by track_number and removing VideoMuxerArg?
Thank you for your review :) This CL is tested with unittest only. @mcasas, Could you let me know which site I can test for this CL? https://codereview.chromium.org/1352243002/diff/60001/content/renderer/media/... File content/renderer/media/media_recorder_handler.h (right): https://codereview.chromium.org/1352243002/diff/60001/content/renderer/media/... content/renderer/media/media_recorder_handler.h:65: // bool is_key_frame); On 2015/10/16 01:11:03, mcasas wrote: > ? Oops. Removed. https://codereview.chromium.org/1352243002/diff/60001/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1352243002/diff/60001/media/capture/webm_muxe... media/capture/webm_muxer.h:93: std::vector<VideoMuxerArg> video_muxer_args_; On 2015/10/16 01:11:03, mcasas wrote: > what about an std::map indexed by track_number and removing VideoMuxerArg? This std::vector is to get the |track_number| and |fist_frame_timestamp| from |track_index|. |track_index| is assigned sequentially, so I use std::vector instead of std::map. |track_number| is not same with |track_index| so I think it's hard to use map indexed by track_number in this case. Please let me know if I miss something.
On 2015/10/21 05:35:51, msu.koo wrote: > Thank you for your review :) > > This CL is tested with unittest only. > @mcasas, Could you let me know which site I can test for this CL? > > https://codereview.chromium.org/1352243002/diff/60001/content/renderer/media/... > File content/renderer/media/media_recorder_handler.h (right): > > https://codereview.chromium.org/1352243002/diff/60001/content/renderer/media/... > content/renderer/media/media_recorder_handler.h:65: // bool > is_key_frame); > On 2015/10/16 01:11:03, mcasas wrote: > > ? > > Oops. Removed. > > https://codereview.chromium.org/1352243002/diff/60001/media/capture/webm_muxer.h > File media/capture/webm_muxer.h (right): > > https://codereview.chromium.org/1352243002/diff/60001/media/capture/webm_muxe... > media/capture/webm_muxer.h:93: std::vector<VideoMuxerArg> video_muxer_args_; > On 2015/10/16 01:11:03, mcasas wrote: > > what about an std::map indexed by track_number and removing VideoMuxerArg? > > This std::vector is to get the |track_number| and |fist_frame_timestamp| from > |track_index|. |track_index| is assigned sequentially, so I use std::vector > instead of std::map. > > |track_number| is not same with |track_index| so I think it's hard to use map > indexed by track_number in this case. Please let me know if I miss something. @mcasas, could you please take a look this patch? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
