Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225123006:
media/capture: Adding WebmMuxer class and unittests (Closed)
|
Created:
5 years, 5 months ago by mcasas Modified:
5 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@m_crbug262211__MSRecorder__2__libwebm_reland_in_third_party Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia/capture: Adding WebmMuxer class and unittests
WebmMuxer gets encoded video packets and pings a
callback with packetised "blobs" following Live WebM
(simplified Matroska container) format.
See DD @ https://goo.gl/kreaQj for the plan and
https://codereview.chromium.org/1211973012/ for a
hack of all parts together.
BUG=262211
Committed: https://crrev.com/6f2bce27d46c0fd56d26143acdfdcdb1a7fc463f
Cr-Commit-Position: refs/heads/master@{#340066}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : emircan@ nits, rebase (only media.gyp affected), removed bool from WriteCallbackCB signature #
Total comments: 25
Patch Set 3 : miu@s comments #
Total comments: 8
Patch Set 4 : miu@s second round of comments #
Total comments: 6
Patch Set 5 : miu@s third round of comments #
Total comments: 14
Patch Set 6 : dalecurtis@ and tomfinegan@ comments #
Total comments: 7
Patch Set 7 : dalecurtis@ second round of comments #
Total comments: 27
Patch Set 8 : dalecurtis@ third round of comments #Patch Set 9 : Removed MockWebmMuxerEventHandler #
Total comments: 2
Patch Set 10 : Using base::StringPiece for encoded_data input #
Messages
Total messages: 54 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org, emircan@chromium.org
dalecurtis@ PTAL or fwd appropriately, thanks! emircan@ PTAL
Video can easily outgrow available memory in a minute or so. I don't think an in memory only approach is viable. Also this probably shouldn't live in media/filters. Have you given thought to mojo service which can write these frames to disk somewhere? Do we have a temporary storage infrastructure in the renderer?
https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... File media/filters/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... media/filters/webm_muxer.cc:52: track_index_starting_from_one) == track_indexes_.end()) { Do you need to keep the order of incoming track_index? If not, std::set would be a better fit than std::vector. https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... media/filters/webm_muxer.cc:101: position_ += len; base::CheckedNumeric would be useful here in case of any overflow.
On 2015/07/13 18:12:48, DaleCurtis wrote: > Video can easily outgrow available memory in a minute or so. I don't think an in > memory only approach is viable. Also this probably shouldn't live in > media/filters. > > Have you given thought to mojo service which can write these frames to disk > somewhere? Do we have a temporary storage infrastructure in the renderer? Video is not stored in C++ code, but passed to renderer as blobs, essentially one header+frame (video and/or audio) at a time. (What JS does with it is beyond Chrome). Marking the stream as live has more to do with the header/ tailer. Where should we put the muxer if not in media/filters?
media/capture? I see that you're using a write callback, but I haven't traced to see who is responsible for handling that. Will it be an IPC or a mojo service?
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:160001) has been deleted
On 2015/07/13 22:20:04, DaleCurtis wrote: > media/capture? media/capture is now populated with screen/tab capture specific code. This muxer can and should be extended for audio and IMHO it doesn't fit there right now -- incidentally, I don't think the code living there fits its name either, hence http://crrev.com/1231863011. filters/ sounds much better to me, f.e: JpegParser - a class for parsing a Jpeg file header and FFmpegDemuxer - a FFmpeg stream container demuxer, are in filters/ now. > I see that you're using a write callback, but I haven't traced > to see who is responsible for handling that. > > Will it be an IPC or a mojo service? Please see the DD, it will call a Blink class (or a family of) generating a JS blob out of it. One frame (and header) at a time.
Patchset #2 (id:180001) has been deleted
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL (notwithstanding dalecurtis@ location comments) https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... File media/filters/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... media/filters/webm_muxer.cc:52: track_index_starting_from_one) == track_indexes_.end()) { On 2015/07/13 18:14:40, emircan wrote: > Do you need to keep the order of incoming track_index? If not, std::set would be > a better fit than std::vector. Done. https://codereview.chromium.org/1225123006/diff/80001/media/filters/webm_muxe... media/filters/webm_muxer.cc:101: position_ += len; On 2015/07/13 18:14:40, emircan wrote: > base::CheckedNumeric would be useful here in case of any overflow. Done.
filters/ is entirely demuxers and media playback code though. I feel like this should live wherever the actual impl for this stuff goes.
https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:27: base::Time::FromUTCString("1 Jan 2001 00:00:00.000", &utc_epoch); BTW--The media/PRESUBMIT.py script may cause commit problems if you use base::Time. An alternative would be: const base::TimeDelta webm_epoch = base::TimeTicks::UnixEpoch() + base::TimeDelta::FromSeconds(978307200); ... info->set_date_utc(base::TimeTicks::Now() - webm_epoch).InMicroseconds() * nanos_per_micro); https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:60: (timestamp - time_start_).InMilliseconds(), InMicroseconds() please. See comment below. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:86: // By default it's is milliseconds, but DCHECK that. BTW--One of my pet peeves is truncating media timestamps to milliseconds. Can we set a microsecond timebase here? https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:97: write_data_callback_.Run(static_cast<const char*>(buf), len); Potential bug: If |len| is greater than 2^31-1, this will translate into a negative number. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:103: return position_.ValueOrDefault(0); Shouldn't this be ValueOrDie()? Also, there's an implicit unsigned-to-signed conversion here, so |position_| should probably be a CheckedNumeric<int64>. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:23: typedef base::Callback<void(const char*, int)> WriteDataCB; Please document when and how this callback will be run. I assume this is an output byte array and a length? Can the length be zero? Does that indicate EOF? https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:28: void OnEncodedVideo(int track_index, This method needs documentation. Below, for AddVideoTrack(), you mention the track index starts from one. In other words, how does a client of this code determine what |track_index| should be? https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:30: const base::TimeTicks& timestamp, The caller should provide the media timestamp as a TimeDelta (where a zero TimeDelta is the timestamp of the first frame). This would be consistent with other code in media/filters. Then, this class wouldn't be responsible for any time math, which could potentially cause flaky behavior since it has a dependency on the system clock. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:32: const gfx::Size& frame_size, What happens when |frame_size| and/or |frame_rate| change between calls (for the same video track)? https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:67: std::set<int> track_indexes_; You don't need this since the same information can be accessed directly from |segment_|. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer_unittest.cc:138: } Consider writing a test that encodes multiple tracks. I'm not sure it's valid to call Segment::AddFrame() until all video tracks have been added. https://codereview.chromium.org/1225123006/diff/190001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1225123006/diff/190001/media/media.gyp#newcode1 media/media.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Don't forget to update BUILD.gn files as well.
BTW--Thanks for doing this! I actually have a need for a WebM muxer right now for helping to test VP8 HW decoder implementations. :)
Patchset #3 (id:210001) has been deleted
miu@ PTAL (dalecurtis@: moved webm_muxer* to media/capture.) https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:27: base::Time::FromUTCString("1 Jan 2001 00:00:00.000", &utc_epoch); On 2015/07/16 01:46:25, miu wrote: > BTW--The media/PRESUBMIT.py script may cause commit problems if you use > base::Time. An alternative would be: > > const base::TimeDelta webm_epoch = base::TimeTicks::UnixEpoch() + > base::TimeDelta::FromSeconds(978307200); > ... > info->set_date_utc(base::TimeTicks::Now() - webm_epoch).InMicroseconds() * > nanos_per_micro); Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:60: (timestamp - time_start_).InMilliseconds(), On 2015/07/16 01:46:25, miu wrote: > InMicroseconds() please. See comment below. Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:86: // By default it's is milliseconds, but DCHECK that. On 2015/07/16 01:46:25, miu wrote: > BTW--One of my pet peeves is truncating media timestamps to milliseconds. Can > we set a microsecond timebase here? Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:97: write_data_callback_.Run(static_cast<const char*>(buf), len); On 2015/07/16 01:46:25, miu wrote: > Potential bug: If |len| is greater than 2^31-1, this will translate into a > negative number. Made them size_t in WriteDataCB etc. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.cc:103: return position_.ValueOrDefault(0); On 2015/07/16 01:46:25, miu wrote: > Shouldn't this be ValueOrDie()? Also, there's an implicit unsigned-to-signed > conversion here, so |position_| should probably be a CheckedNumeric<int64>. Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:23: typedef base::Callback<void(const char*, int)> WriteDataCB; On 2015/07/16 01:46:26, miu wrote: > Please document when and how this callback will be run. I assume this is an > output byte array and a length? Can the length be zero? Does that indicate > EOF? Done. Clarified a bit the WebM restrictions in the class header; most notably there's no container Trailer, so there's no EOF indication "per-se". https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:28: void OnEncodedVideo(int track_index, On 2015/07/16 01:46:25, miu wrote: > This method needs documentation. Below, for AddVideoTrack(), you mention the > track index starts from one. In other words, how does a client of this code > determine what |track_index| should be? Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:30: const base::TimeTicks& timestamp, On 2015/07/16 01:46:26, miu wrote: > The caller should provide the media timestamp as a TimeDelta (where a zero > TimeDelta is the timestamp of the first frame). This would be consistent with > other code in media/filters. Then, this class wouldn't be responsible for any > time math, which could potentially cause flaky behavior since it has a > dependency on the system clock. Done. I like it. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:32: const gfx::Size& frame_size, On 2015/07/16 01:46:26, miu wrote: > What happens when |frame_size| and/or |frame_rate| change between calls (for the > same video track)? As per offline discussion, what happens is that: a) the first time a track_index is seen, |frame_size| and |frame_rate| are added as indications to it but b) each individual encoded video frame header (SimpleBlock) has no indication of size and/or fps, being transcribed in the VPx header itself. Added to the method doco. Perhaps it would be more interesting to publish CreateVideoTrack() and let the clients specify those params ahead of OnEncodedVideo(), and change this last one's signature to void OnEncodedVideo(int track_number, const std::string& encoded_data, const base::TimeDelta& timedelta_us, bool keyframe); WDYT? https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer.h:67: std::set<int> track_indexes_; On 2015/07/16 01:46:26, miu wrote: > You don't need this since the same information can be accessed directly from > |segment_|. Done. https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer_unittest.cc:138: } On 2015/07/16 01:46:26, miu wrote: > Consider writing a test that encodes multiple tracks. I'm not sure it's valid > to call Segment::AddFrame() until all video tracks have been added. It is possible, I think Segment seems to output then a new Track info header, even if in the middle of the stream. From the unofficial doc [1]. "All tracks shall be described in one (or more, but preferably only one) TRACKS element." Any ideas on how to test it? :) (Short of compiling the whole libwebm and demuxing everything...) [1] http://www.matroska.org/files/matroska.pdf https://codereview.chromium.org/1225123006/diff/190001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1225123006/diff/190001/media/media.gyp#newcode1 media/media.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/07/16 01:46:26, miu wrote: > Don't forget to update BUILD.gn files as well. Done.
Patchset #3 (id:230001) has been deleted
https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... File media/filters/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/190001/media/filters/webm_mux... media/filters/webm_muxer_unittest.cc:138: } On 2015/07/17 17:45:07, mcasas wrote: > On 2015/07/16 01:46:26, miu wrote: > > Consider writing a test that encodes multiple tracks. I'm not sure it's valid > > to call Segment::AddFrame() until all video tracks have been added. > > It is possible, I think Segment seems to output then a new Track info header, > even if in the middle of the stream. From the unofficial doc [1]. > "All tracks shall be described in one (or more, but preferably only one) > TRACKS element." > Any ideas on how to test it? :) > (Short of compiling the whole libwebm and demuxing everything...) > > [1] http://www.matroska.org/files/matroska.pdf Weird. I guess no need to test that you can't do something the library let's you do. Note, however, if you take my advice on making AddVideoTrack() public, this should be a non-issue. ;-) https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.cc:101: segment_->GetSegmentInfo()->set_timecode_scale(1000000000ull); IIUC, this be 1000 instead? Better yet: base::Time::kNanosecondsPerMicrosecond. https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.h:49: const base::TimeDelta& timedelta_us, naming: No _us suffix here, since that's an abstraction violation. https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.h:51: const gfx::Size& frame_size, Instead of placing a burden on the caller to pass the exact same |frame_size| and |frame_rate| args each time, why not get rid of these args and just make AddVideoTrack() a public method that must be called once before OnEncodedVideo() is ever called? https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:88: base::RunLoop().RunUntilIdle(); IIUC, you don't need any of these base::RunLoop().RunUntilIdle() statements in any of your tests since no tasks are being posted to a loop.
Patchset #4 (id:270001) has been deleted
PTAL https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.cc:101: segment_->GetSegmentInfo()->set_timecode_scale(1000000000ull); On 2015/07/17 22:23:18, miu wrote: > IIUC, this be 1000 instead? Better yet: base::Time::kNanosecondsPerMicrosecond. Yeah, done. https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.h:49: const base::TimeDelta& timedelta_us, On 2015/07/17 22:23:19, miu wrote: > naming: No _us suffix here, since that's an abstraction violation. Done. https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer.h:51: const gfx::Size& frame_size, On 2015/07/17 22:23:19, miu wrote: > Instead of placing a burden on the caller to pass the exact same |frame_size| > and |frame_rate| args each time, why not get rid of these args and just make > AddVideoTrack() a public method that must be called once before OnEncodedVideo() > is ever called? Yeps, exactly my question before, got lost in the rebase :-) https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/250001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:88: base::RunLoop().RunUntilIdle(); On 2015/07/17 22:23:19, miu wrote: > IIUC, you don't need any of these base::RunLoop().RunUntilIdle() statements in > any of your tests since no tasks are being posted to a loop. True, Done.
https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.cc:84: const base::TimeDelta& timedelta_us, naming: Remove _us suffix here too, and this should be called timestamp or media_timestamp. https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.h:52: // |timedelta| is a delta from UTC/WebM Epoch, 2001-01-01 00:00:00. Don't burden the client with the WebM epoch calculations. Instead, and for consistency with all other media code, this should be the media timestamp where frame zero has a TimeDelta = zero. (The next frame has a timestamp = 1sec / frame_rate, and so on.) If, internally, the timestamp has to be converted to some kind of relative-to-WebM-epoch timestamp, you should do that translation inside OnEncodedVideo(). It's not clear to me what mkvmuxer needs here. I'll leave that to you to figure out. I noticed you're not setting info->set_date_utc() anymore? If the date info is irrelevant, perhaps you can just pass the timestamps directly into the container w/o any relative-to-date translation? https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.h:55: const base::TimeDelta& timedelta, naming: Forgot to mention last round: s/timedelta/timestamp/ or s/timedelta/media_timestamp/
mcasas@chromium.org changed reviewers: + tomfinegan@chromium.org
miu@ PTAL tomfinegan@ could you have a look at my usage of libwwebm please? particularly if there's anything standing out in timestamp usage. https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.cc:84: const base::TimeDelta& timedelta_us, On 2015/07/18 00:29:39, miu wrote: > naming: Remove _us suffix here too, and this should be called timestamp or > media_timestamp. Done. https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.h:52: // |timedelta| is a delta from UTC/WebM Epoch, 2001-01-01 00:00:00. On 2015/07/18 00:29:39, miu wrote: > Don't burden the client with the WebM epoch calculations. Instead, and for > consistency with all other media code, this should be the media timestamp where > frame zero has a TimeDelta = zero. (The next frame has a timestamp = 1sec / > frame_rate, and so on.) If, internally, the timestamp has to be converted to > some kind of relative-to-WebM-epoch timestamp, you should do that translation > inside OnEncodedVideo(). > Done. > It's not clear to me what mkvmuxer needs here. I'll leave that to you to figure > out. I noticed you're not setting info->set_date_utc() anymore? If the date > info is irrelevant, perhaps you can just pass the timestamps directly into the > container w/o any relative-to-date translation? I think mkvmuxer only needs a monotonously increasing timestamp, and is used for audio/video/text synch, especially for Live streams. https://codereview.chromium.org/1225123006/diff/290001/media/capture/webm_mux... media/capture/webm_muxer.h:55: const base::TimeDelta& timedelta, On 2015/07/18 00:29:39, miu wrote: > naming: Forgot to mention last round: s/timedelta/timestamp/ or > s/timedelta/media_timestamp/ Done.
Patchset #5 (id:310001) has been deleted
What is the binary size increase for using libwebm too? https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:33: DVLOG(2) << __FUNCTION__; Remove DVLOG() of this type? Even at (2) this isn't super useful I think. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:50: void WebmMuxer::AddVideoTrack(uint32_t track_number, Why do you need to expose the track number? Just keep an internal counter which starts at one instead and return it from this function? https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:58: friend class WebmMuxerTest; Why both this and the FRIEND_TEST.... https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:82: const scoped_ptr<mkvmuxer::Segment> segment_; Why const scoped_ptr? Just use a mkvmuxer::Segment directly here?
Looks fine excepting your timecode scale, and using the webm blessed value isn't going to change much. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:79: base::Time::kNanosecondsPerMicrosecond); http://www.webmproject.org/docs/container/ Since you're a webm muxing app, you want a timecode scale of 1,000,000 because we're good citizens and follow the guidelines... (right?) :) https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:97: timestamp.InMicroseconds(), You'll need to scale this for the new timecode scale value.
guys PTAL https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:33: DVLOG(2) << __FUNCTION__; On 2015/07/20 18:45:17, DaleCurtis wrote: > Remove DVLOG() of this type? Even at (2) this isn't super useful I think. Done. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:50: void WebmMuxer::AddVideoTrack(uint32_t track_number, On 2015/07/20 18:45:17, DaleCurtis wrote: > Why do you need to expose the track number? Just keep an internal counter which > starts at one instead and return it from this function? Even better: if the caller doesn't decide on the track index, I can let the library decide for me by passing a 0 as track index to AddVideoTrack(). https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:79: base::Time::kNanosecondsPerMicrosecond); On 2015/07/20 18:58:23, Tom Finegan wrote: > http://www.webmproject.org/docs/container/ > > Since you're a webm muxing app, you want a timecode scale of 1,000,000 because > we're good citizens and follow the guidelines... (right?) :) > Done. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.cc:97: timestamp.InMicroseconds(), On 2015/07/20 18:58:23, Tom Finegan wrote: > You'll need to scale this for the new timecode scale value. Done. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:58: friend class WebmMuxerTest; On 2015/07/20 18:45:17, DaleCurtis wrote: > Why both this and the FRIEND_TEST.... Both are needed for the individual tests to access private members (note that tests are subclasses). I figured is better to add the macro than add an accessor, that increases the whole class footprint. https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:82: const scoped_ptr<mkvmuxer::Segment> segment_; On 2015/07/20 18:45:17, DaleCurtis wrote: > Why const scoped_ptr? Just use a mkvmuxer::Segment directly here? Done.
Also, binary size data? :) https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:58: friend class WebmMuxerTest; On 2015/07/20 20:15:20, mcasas wrote: > On 2015/07/20 18:45:17, DaleCurtis wrote: > > Why both this and the FRIEND_TEST.... > > Both are needed for the individual tests to access > private members (note that tests are subclasses). > I figured is better to add the macro than add an > accessor, that increases the whole class footprint. Just add accessors to the WebmMuxerTest harness. I loath FRIEND_TEST_ALL :) https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:29: mkvmuxer::uint64 WebmMuxer::AddVideoTrack(const gfx::Size& frame_size, Since this is the Chrome side interface, can we just return uint64_t? Ditto for the OnEncodedVideo. We should try to avoid the spread of these outside of this class. https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:57: const std::string& encoded_data, Do you want std::string& here? Should you instead be passing a ptr+size or something like a DecoderBuffer / RefCountedMemory? We're going to want to minimize copies along this path likely.
dalecurtis@ PTAL. About the binary size: "libwebm binary size only includes muxing parts, 12KB in Debug, 10KB in Release." (in Linux box). (it's also in the DD https://goo.gl/kreaQj and in the libwebm landing CL) https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/330001/media/capture/webm_mux... media/capture/webm_muxer.h:58: friend class WebmMuxerTest; On 2015/07/20 20:23:07, DaleCurtis wrote: > On 2015/07/20 20:15:20, mcasas wrote: > > On 2015/07/20 18:45:17, DaleCurtis wrote: > > > Why both this and the FRIEND_TEST.... > > > > Both are needed for the individual tests to access > > private members (note that tests are subclasses). > > I figured is better to add the macro than add an > > accessor, that increases the whole class footprint. > > Just add accessors to the WebmMuxerTest harness. I loath FRIEND_TEST_ALL :) Done. https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:29: mkvmuxer::uint64 WebmMuxer::AddVideoTrack(const gfx::Size& frame_size, On 2015/07/20 20:23:07, DaleCurtis wrote: > Since this is the Chrome side interface, can we just return uint64_t? Ditto for > the OnEncodedVideo. We should try to avoid the spread of these outside of this > class. Done. https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:57: const std::string& encoded_data, On 2015/07/20 20:23:07, DaleCurtis wrote: > Do you want std::string& here? Should you instead be passing a ptr+size or > something like a DecoderBuffer / RefCountedMemory? We're going to want to > minimize copies along this path likely. Hmm at this point I'm happy with std::string&, reasons: - http://crrev.com/1233033002 VideoTrackRecorder, libvpx decode likes to "add" encoded data into a buffer, and a growing buffer suits an std::string. To make it more evident, I changed to buffer ptr and size. - I played around with RefCountedString::TakeString() and it didn't clarify much the mechanism. - Encode and Muxing happen in a single thread and seems reasonable that this will be the normal case, so encode-mux is a one-to-one synchronous call, hence no need for ref counting etc. All of this notwithstanding that when we bring the whole shebang together, we might feel like some extra optimisation. WDYT?
Patchset #7 (id:370001) has been deleted
Thanks for the binary size info; I didn't see it in the libwebm CL (I still don't...). https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:57: const std::string& encoded_data, On 2015/07/21 12:56:20, mcasas wrote: > On 2015/07/20 20:23:07, DaleCurtis wrote: > > Do you want std::string& here? Should you instead be passing a ptr+size or > > something like a DecoderBuffer / RefCountedMemory? We're going to want to > > minimize copies along this path likely. > > Hmm at this point I'm happy with std::string&, reasons: > - http://crrev.com/1233033002 VideoTrackRecorder, > libvpx decode likes to "add" encoded data into a buffer, > and a growing buffer suits an std::string. To make it > more evident, I changed to buffer ptr and size. > - I played around with RefCountedString::TakeString() > and it didn't clarify much the mechanism. > - Encode and Muxing happen in a single thread and > seems reasonable that this will be the normal case, > so encode-mux is a one-to-one synchronous call, > hence no need for ref counting etc. > All of this notwithstanding that when we bring the > whole shebang together, we might feel like some > extra optimisation. > > WDYT? I didn't realize that about the vp8 encode api, so I defer to Tom as to the best way of moving these bits around post-encoding. Mostly I'd worry about unintentional copies sneaking in (use w/ Bind(), lost const&); you could use StringPiece for a stronger guarantee. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:75: return 0; Is this the right return value, generally a Write() returns the number of bytes written. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:83: // The stream is not Seekable() so indicate we cannot set the position. Does NOTREACHED() get hit? https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:95: << "Can't go back in a Live Mkv stream."; live webm stream https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.h:38: typedef base::Callback<void(const char*, size_t)> WriteDataCB; Switch to C++11 "using" https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.h:54: const base::TimeDelta& timestamp, Pass TimeDelta by value, not reference. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:24: virtual void WriteCallback(const char* data, size_t len) {} = 0; https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:51: protected: Just leave everything public, no need to worry about this in a test. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:55: base::Bind(&MockWebmMuxerEventHandler::WriteCallback, Why bind to a whole new class? Just add the MOCK_METHOD2() to this class. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:60: void SetUp() override { Choose SetUp() or a constructor, no need for both. Put the constructor at the top of the class too. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:78: const scoped_refptr<MockWebmMuxerEventHandler> mock_handler_; Why is this ref counted? Is this to deal with the fact that WebmMuxer won't finalize until it's destructed? I think you should expose a Finalize() API then. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:79: const scoped_ptr<WebmMuxer> webm_muxer_; No scoped_ptr, just stack allocate. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:87: // Checks that AddVideoTrack adds a Track. Newline. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:91: EXPECT_NE(nullptr, GetWebmMuxerSegment().GetTrackByNumber(track_number)); EXPECT_TRUE
PTAL https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:57: const std::string& encoded_data, On 2015/07/21 16:52:30, DaleCurtis wrote: > On 2015/07/21 12:56:20, mcasas wrote: > > On 2015/07/20 20:23:07, DaleCurtis wrote: > > > Do you want std::string& here? Should you instead be passing a ptr+size or > > > something like a DecoderBuffer / RefCountedMemory? We're going to want to > > > minimize copies along this path likely. > > > > Hmm at this point I'm happy with std::string&, reasons: > > - http://crrev.com/1233033002 VideoTrackRecorder, > > libvpx decode likes to "add" encoded data into a buffer, > > and a growing buffer suits an std::string. To make it > > more evident, I changed to buffer ptr and size. > > - I played around with RefCountedString::TakeString() > > and it didn't clarify much the mechanism. > > - Encode and Muxing happen in a single thread and > > seems reasonable that this will be the normal case, > > so encode-mux is a one-to-one synchronous call, > > hence no need for ref counting etc. > > All of this notwithstanding that when we bring the > > whole shebang together, we might feel like some > > extra optimisation. > > > > WDYT? > > I didn't realize that about the vp8 encode api, so I defer to Tom as to the best > way of moving these bits around post-encoding. Mostly I'd worry about > unintentional copies sneaking in (use w/ Bind(), lost const&); you could use > StringPiece for a stronger guarantee. > Acknowledged. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:75: return 0; On 2015/07/21 16:52:30, DaleCurtis wrote: > Is this the right return value, generally a Write() returns the number of bytes > written. [1] "returns 0 on success" [1] https://code.google.com/p/webm/source/browse/mkvmuxer.hpp?repo=libwebm  https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:83: // The stream is not Seekable() so indicate we cannot set the position. On 2015/07/21 16:52:30, DaleCurtis wrote: > Does NOTREACHED() get hit? I doubt it, because the stream is "Live". Returning 0 would mean success. [1] [1] https://code.google.com/p/webm/source/browse/mkvmuxer.hpp?repo=libwebm' https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.cc:95: << "Can't go back in a Live Mkv stream."; On 2015/07/21 16:52:30, DaleCurtis wrote: > live webm stream Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.h:38: typedef base::Callback<void(const char*, size_t)> WriteDataCB; On 2015/07/21 16:52:30, DaleCurtis wrote: > Switch to C++11 "using" Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer.h:54: const base::TimeDelta& timestamp, On 2015/07/21 16:52:31, DaleCurtis wrote: > Pass TimeDelta by value, not reference. Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:24: virtual void WriteCallback(const char* data, size_t len) {} On 2015/07/21 16:52:31, DaleCurtis wrote: > = 0; Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:51: protected: On 2015/07/21 16:52:31, DaleCurtis wrote: > Just leave everything public, no need to worry about this in a test. Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:55: base::Bind(&MockWebmMuxerEventHandler::WriteCallback, On 2015/07/21 16:52:31, DaleCurtis wrote: > Why bind to a whole new class? Just add the MOCK_METHOD2() to this class. Then I'd need to use base::Unretained since this class is not reference counted. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:60: void SetUp() override { On 2015/07/21 16:52:31, DaleCurtis wrote: > Choose SetUp() or a constructor, no need for both. Put the constructor at the > top of the class too. Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:78: const scoped_refptr<MockWebmMuxerEventHandler> mock_handler_; On 2015/07/21 16:52:31, DaleCurtis wrote: > Why is this ref counted? Is this to deal with the fact that WebmMuxer won't > finalize until it's destructed? I think you should expose a Finalize() API > then. It's to Bind() the callback. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:79: const scoped_ptr<WebmMuxer> webm_muxer_; On 2015/07/21 16:52:31, DaleCurtis wrote: > No scoped_ptr, just stack allocate. Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:87: // Checks that AddVideoTrack adds a Track. On 2015/07/21 16:52:31, DaleCurtis wrote: > Newline. Done. https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:91: EXPECT_NE(nullptr, GetWebmMuxerSegment().GetTrackByNumber(track_number)); On 2015/07/21 16:52:31, DaleCurtis wrote: > EXPECT_TRUE Done.
https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1225123006/diff/390001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:55: base::Bind(&MockWebmMuxerEventHandler::WriteCallback, On 2015/07/21 19:47:00, mcasas wrote: > On 2015/07/21 16:52:31, DaleCurtis wrote: > > Why bind to a whole new class? Just add the MOCK_METHOD2() to this class. > > Then I'd need to use base::Unretained since this class > is not reference counted. That's fine, we use that all over the place and since you own the objects in question it's a valid use.
https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... media/capture/webm_muxer.cc:57: const std::string& encoded_data, On 2015/07/21 12:56:20, mcasas wrote: > On 2015/07/20 20:23:07, DaleCurtis wrote: > > Do you want std::string& here? Should you instead be passing a ptr+size or > > something like a DecoderBuffer / RefCountedMemory? We're going to want to > > minimize copies along this path likely. > > Hmm at this point I'm happy with std::string&, reasons: > - http://crrev.com/1233033002 VideoTrackRecorder, > libvpx decode likes to "add" encoded data into a buffer, > and a growing buffer suits an std::string. To make it > more evident, I changed to buffer ptr and size. By default, this is true, but you can make the library encode into your buffers (and decode, but that's a typo, right?). The encoding app uses vpx_codec_set_cx_data_buf() to give the encoder buffers, calls vpx_codec_encode(), then calls vpx_codec_get_data() (i.e. almost the same as what you have now, just with the extra call to pass in your buffer). I don't think it's necessary to do this now, but maybe in a future optimization pass it would be worthwhile. Since you're dealing with compressor output the buffers aren't going to be very big anyway. As Knuth said, premature optimization is the root of all evil. It's probably too early a stage to be worrying about this copy. Otoh, you guys might feel differently. The thing to keep in mind is requested datarate vs frame rate. For example, 1 megabit at 15 fps you end up with an average frame size of just 8.3k. Is jumping through hoops to save that one copy worth it yet? Also, I'm hesitant to recommend the use of vpx_codec_set_cx_data_buf(). It doesn't have test coverage, and I have no knowledge of its use in the wild. > - I played around with RefCountedString::TakeString() > and it didn't clarify much the mechanism. > - Encode and Muxing happen in a single thread and > seems reasonable that this will be the normal case, > so encode-mux is a one-to-one synchronous call, > hence no need for ref counting etc. > All of this notwithstanding that when we bring the > whole shebang together, we might feel like some > extra optimisation. > > WDYT?
On 2015/07/21 21:19:09, Tom Finegan wrote: > https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... > File media/capture/webm_muxer.cc (right): > > https://codereview.chromium.org/1225123006/diff/350001/media/capture/webm_mux... > media/capture/webm_muxer.cc:57: const std::string& encoded_data, > On 2015/07/21 12:56:20, mcasas wrote: > > On 2015/07/20 20:23:07, DaleCurtis wrote: > > > Do you want std::string& here? Should you instead be passing a ptr+size or > > > something like a DecoderBuffer / RefCountedMemory? We're going to want to > > > minimize copies along this path likely. > > > > Hmm at this point I'm happy with std::string&, reasons: > > - http://crrev.com/1233033002 VideoTrackRecorder, > > libvpx decode likes to "add" encoded data into a buffer, > > and a growing buffer suits an std::string. To make it > > more evident, I changed to buffer ptr and size. > > By default, this is true, but you can make the library encode into your buffers > (and decode, but that's a typo, right?). The encoding app uses > vpx_codec_set_cx_data_buf() to give the encoder buffers, calls > vpx_codec_encode(), then calls vpx_codec_get_data() (i.e. almost the same as > what you have now, just with the extra call to pass in your buffer). > > I don't think it's necessary to do this now, but maybe in a future optimization > pass it would be worthwhile. Since you're dealing with compressor output the > buffers aren't going to be very big anyway. As Knuth said, premature > optimization is the root of all evil. It's probably too early a stage to be > worrying about this copy. > > Otoh, you guys might feel differently. The thing to keep in mind is requested > datarate vs frame rate. For example, 1 megabit at 15 fps you end up with an > average frame size of just 8.3k. Is jumping through hoops to save that one copy > worth it yet? > > Also, I'm hesitant to recommend the use of vpx_codec_set_cx_data_buf(). It > doesn't have test coverage, and I have no knowledge of its use in the wild. > > > - I played around with RefCountedString::TakeString() > > and it didn't clarify much the mechanism. > > - Encode and Muxing happen in a single thread and > > seems reasonable that this will be the normal case, > > so encode-mux is a one-to-one synchronous call, > > hence no need for ref counting etc. > > All of this notwithstanding that when we bring the > > whole shebang together, we might feel like some > > extra optimisation. > > > > WDYT? From my perspective this is l-g-t-m, but I don't want to go approving changes in parts of the tree that I don't own.
https://codereview.chromium.org/1225123006/diff/430001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/430001/media/capture/webm_mux... media/capture/webm_muxer.h:38: using WriteDataCB = base::Callback<void(const char*, size_t)>; You use char* here and uint8_t* below (and void* on the Write interface, but that's private so it's not a big deal); it's good practice to pick one. base::StringPiece might be a better choice too since it has a baked in size and data pointer.
dalecurtis@chromium.org changed required reviewers: + miu@chromium.org
lgtm % nits, but please get signoff from miu@ since he reviewed earlier.
lgtm
https://codereview.chromium.org/1225123006/diff/430001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1225123006/diff/430001/media/capture/webm_mux... media/capture/webm_muxer.h:38: using WriteDataCB = base::Callback<void(const char*, size_t)>; On 2015/07/22 18:19:17, DaleCurtis wrote: > You use char* here and uint8_t* below (and void* on the Write interface, but > that's private so it's not a big deal); it's good practice to pick one. > base::StringPiece might be a better choice too since it has a baked in size and > data pointer. using base::StringPiece now.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1225123006/#ps450001 (title: "Using base::StringPiece for encoded_data input")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225123006/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1225123006/450001
Message was sent while issue was closed.
Committed patchset #10 (id:450001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6f2bce27d46c0fd56d26143acdfdcdb1a7fc463f Cr-Commit-Position: refs/heads/master@{#340066} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
