Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10784037:
[cros] Implement WebM encoder/muxer for animated avatar capture. (Closed)
|
Created:
8 years, 5 months ago by Ivan Korotkov Modified:
8 years, 5 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[cros] Implement WebM encoder/muxer for animated avatar capture.
BUG=132423
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148024
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move WebmEncoder to chrome/browser/chromeos #Patch Set 3 : Review fixes #
Total comments: 10
Patch Set 4 : Move code to media/webm/chromeos #
Total comments: 59
Patch Set 5 : Review fixes #
Total comments: 8
Patch Set 6 : Merge #Patch Set 7 : Review fixes #
Total comments: 9
Patch Set 8 : Review fixes #
Messages
Total messages: 21 (0 generated)
PTAL Won't work until http://codereview.chromium.org/10793025/ is submitted.
John, could you please also review the VP8/WebM related code?
just a few nits, looks good overall. http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:80: vpx_image_t image; Can simplify this block to: vpx_img_wrap(&image, VPX_IMG_FMT_I420, width_, height_, 1, yuv_frame_.get()) http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:125: y, width_, if you want, you can remove all these variables and knowledge of the yuv internals (plane offsets, strides, etc) from this function by pulling the data from the image descriptor set up by vpx_img_wrap() http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:196: static_cast<uint64_t>(fps_.den) / fps_.num * packet->data.frame.pts; multiply by pts before the divide.
http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... File chrome/browser/chromeos/login/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:80: vpx_image_t image; On 2012/07/17 18:33:34, jkoleszar wrote: > Can simplify this block to: > > vpx_img_wrap(&image, VPX_IMG_FMT_I420, width_, height_, 1, yuv_frame_.get()) Cool, thanks. http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:125: y, width_, On 2012/07/17 18:33:34, jkoleszar wrote: > if you want, you can remove all these variables and knowledge of the yuv > internals (plane offsets, strides, etc) from this function by pulling the data > from the image descriptor set up by vpx_img_wrap() Makes sense. http://codereview.chromium.org/10784037/diff/1/chrome/browser/chromeos/login/... chrome/browser/chromeos/login/webm_encoder.cc:196: static_cast<uint64_t>(fps_.den) / fps_.num * packet->data.frame.pts; On 2012/07/17 18:33:34, jkoleszar wrote: > multiply by pts before the divide. Done.
lgtm
Not sure if chrome/browser/chromeos/* is the correct place for these files. /media/webm/?
Moved code to media/webm/chromeos Adding scherkus for OWNERS approval
http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... File chrome/browser/chromeos/webm_encoder.h (right): http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:36: // |fps_n/fps_d|. Must be called on a thread that allows IO. nit: allows disk IO http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:40: // Write global WebM header and starts a single video track. nit: Writes http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:42: // Writes VPX packet to output file. nit: insert empty line http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:44: // Finished video track and closes output file. nit: insert empty line nit: Finishes http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:49: // Closes current top-level sub-element. nit: insert empty line
http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:30: void Ebml_SerializeUnsigned32(EbmlGlobal *ebml, nit: EbmlGlobal* ebml http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:86: config_.g_pass = VPX_RC_ONE_PASS; nit: align comments if possible http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:96: DCHECK_EQ(VPX_CODEC_OK, ret); Pass error to upper level if that fails http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:117: DCHECK_EQ(0, res); Pass error to upper level if that fails? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:121: DCHECK_EQ(VPX_CODEC_OK, ret); Same here. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:141: StartSubElement(EBML); { How about StartSubElement(EBML); { ... } EndSubElement(); formatting? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:150: // Single segment with a video track. nit: insert extra line http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:151: StartSubElement(Segment); { nit: fix formatting as suggested http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:156: StartSubElement(Tracks); { nit: fix formatting as suggested http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:228: fseek(output_, start_pos, SEEK_SET); Check return value? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:232: fseek(output_, end_pos, SEEK_SET); Check return value? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:238: CHECK_EQ(len, ret); Not sure that it makes sense to have a CHECK here. How about if encoding operation fails let caller know about it and handle on that level. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:262: NOTREACHED(); nit: Add some error message?
http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/DEPS File media/webm/chromeos/DEPS (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/DEPS#ne... media/webm/chromeos/DEPS:2: "+libyuv", this rule doesn't look right -- libyuv is under third_party http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... File media/webm/chromeos/ebml_writer.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... media/webm/chromeos/ebml_writer.cc:13: void MEDIA_EXPORT Ebml_Write(EbmlGlobal *glob, pointers w/ types here + below http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... File media/webm/chromeos/ebml_writer.h (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... media/webm/chromeos/ebml_writer.h:13: base::Callback<void(const void* buffer, unsigned long len)> write_cb; uint32 (or even better -- plain old int if it works for you!) http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... media/webm/chromeos/ebml_writer.h:14: base::Callback<void(const void* buffer, int buffer_size, unsigned long len)> ditto http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:10: #include "libyuv/convert.h" shouldn't this be third_party/libyuv/...? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:18: #include "third_party/libvpx/source/libvpx/libmkv/EbmlIDs.h" what's going on here? is there a particular header that causes grief? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:26: const int kNumEncoderThreads = 2; the rest of the media codebase uses static over anonymous namespaces here + function below http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:30: void Ebml_SerializeUnsigned32(EbmlGlobal *ebml, pointers go with types EbmlGlobal* embl http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:33: unsigned char size_serialized = 4 | 0x80; prefer uint8 over unsigned keyword http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:51: ebml_writer_.write_cb = base::Bind( what happens if I use two of these classes at the same time? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:60: DCHECK(!sprite.isNull() && !sprite.empty()); nit: separate the DCHECKs http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:187: unsigned long block_length = (packet->data.frame.sz + 4) | 0x10000000; what's 0x10000000? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:190: unsigned char track_number = 1 | 0x80; what's 0x80? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:197: flags |= 0x80; are there no constants for flags? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:215: uint64_t unknown_len = 0x01FFFFFFFFFFFFFFLLU; no constant? http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:243: for (unsigned long i = len; i-- > 0; ) { nit: let's try to keep conditions as they are and put the iterator modifying stuff in the third clause for (i = len - 1; i >= 0; --i) { // ... } http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.h (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.h:36: WebmEncoder(const FilePath& output_path, unsigned bitrate, bool realtime); you can use int for bitrate -- we do elsewhere in the media code http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.h:85: }; DISALLOW etc...
http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... File chrome/browser/chromeos/webm_encoder.h (right): http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:36: // |fps_n/fps_d|. Must be called on a thread that allows IO. On 2012/07/18 16:23:05, Nikita Kostylev wrote: > nit: allows disk IO Done. http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:40: // Write global WebM header and starts a single video track. On 2012/07/18 16:23:05, Nikita Kostylev wrote: > nit: Writes Done. http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:42: // Writes VPX packet to output file. On 2012/07/18 16:23:05, Nikita Kostylev wrote: > nit: insert empty line Done. http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:44: // Finished video track and closes output file. On 2012/07/18 16:23:05, Nikita Kostylev wrote: > nit: insert empty line > nit: Finishes Done. http://codereview.chromium.org/10784037/diff/5002/chrome/browser/chromeos/web... chrome/browser/chromeos/webm_encoder.h:49: // Closes current top-level sub-element. On 2012/07/18 16:23:05, Nikita Kostylev wrote: > nit: insert empty line Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:30: void Ebml_SerializeUnsigned32(EbmlGlobal *ebml, On 2012/07/19 08:37:12, Nikita Kostylev wrote: > nit: EbmlGlobal* ebml Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:86: config_.g_pass = VPX_RC_ONE_PASS; On 2012/07/19 08:37:12, Nikita Kostylev wrote: > nit: align comments if possible Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:96: DCHECK_EQ(VPX_CODEC_OK, ret); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Pass error to upper level if that fails Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:117: DCHECK_EQ(0, res); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Pass error to upper level if that fails? Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:121: DCHECK_EQ(VPX_CODEC_OK, ret); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Same here. Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:141: StartSubElement(EBML); { On 2012/07/19 08:37:12, Nikita Kostylev wrote: > How about > > StartSubElement(EBML); > { > ... > } > EndSubElement(); > > formatting? IMO that looks against Chrome's style. This one follows if/else and do/while look. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:150: // Single segment with a video track. On 2012/07/19 08:37:12, Nikita Kostylev wrote: > nit: insert extra line Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:228: fseek(output_, start_pos, SEEK_SET); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Check return value? Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:232: fseek(output_, end_pos, SEEK_SET); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Check return value? Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:238: CHECK_EQ(len, ret); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > Not sure that it makes sense to have a CHECK here. How about if encoding > operation fails let caller know about it and handle on that level. It's difficult to do with EbmlXxx interface. Maybe it makes sense to simply LOG(ERROR) in this case, because it's pretty rare (disk full, for example). http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:262: NOTREACHED(); On 2012/07/19 08:37:12, Nikita Kostylev wrote: > nit: Add some error message? Done.
http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/DEPS File media/webm/chromeos/DEPS (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/DEPS#ne... media/webm/chromeos/DEPS:2: "+libyuv", On 2012/07/19 21:46:03, scherkus wrote: > this rule doesn't look right -- libyuv is under third_party The rule matches the #include "libyuv/..." directive, third_party/libyuv wouldn't work. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... File media/webm/chromeos/ebml_writer.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... media/webm/chromeos/ebml_writer.cc:13: void MEDIA_EXPORT Ebml_Write(EbmlGlobal *glob, On 2012/07/19 21:46:03, scherkus wrote: > pointers w/ types here + below Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... File media/webm/chromeos/ebml_writer.h (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/ebml_wr... media/webm/chromeos/ebml_writer.h:13: base::Callback<void(const void* buffer, unsigned long len)> write_cb; On 2012/07/19 21:46:03, scherkus wrote: > uint32 (or even better -- plain old int if it works for you!) Actually I'd prefer to keep this in sync with EbmlWrite/EbmlSerialize declarations in EbmlWriter.h. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:10: #include "libyuv/convert.h" On 2012/07/19 21:46:03, scherkus wrote: > shouldn't this be third_party/libyuv/...? libyuv.gyp adds third_party/libyuv/include/ to -D, so this is ok: https://cs.corp.google.com/#chrome/src/third_party/libjingle/source/talk/sess... Or it could be #include "third_party/libyuv/include/libyuv/..." http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:18: #include "third_party/libvpx/source/libvpx/libmkv/EbmlIDs.h" On 2012/07/19 21:46:03, scherkus wrote: > what's going on here? > > is there a particular header that causes grief? One of functions in EbmlWriter.h has argument named "private" :) http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:26: const int kNumEncoderThreads = 2; On 2012/07/19 21:46:03, scherkus wrote: > the rest of the media codebase uses static over anonymous namespaces here + > function below Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:33: unsigned char size_serialized = 4 | 0x80; On 2012/07/19 21:46:03, scherkus wrote: > prefer uint8 over unsigned keyword Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:51: ebml_writer_.write_cb = base::Bind( On 2012/07/19 21:46:03, scherkus wrote: > what happens if I use two of these classes at the same time? What's the problem with this? Each instance would pass its own ebml_writer_ context to EbmlXxx functions, so its own callbacks will be called. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:187: unsigned long block_length = (packet->data.frame.sz + 4) | 0x10000000; On 2012/07/19 21:46:03, scherkus wrote: > what's 0x10000000? I guess these higher bit masks (0x10000000, 0x80, etc) denote the length of the following field in EBML or something like that. The code comes from libvpx. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:197: flags |= 0x80; On 2012/07/19 21:46:03, scherkus wrote: > are there no constants for flags? libvpx/webrtc use these as is, so I assume not. What's interesting is that EBML doesn't define anything for bit 7 ("unused"), I guess using it as keyframe flag is some sort of WebM extension. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:215: uint64_t unknown_len = 0x01FFFFFFFFFFFFFFLLU; On 2012/07/19 21:46:03, scherkus wrote: > no constant? Same here. I think it's "-1" EBML-encoded. Made it static const. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:243: for (unsigned long i = len; i-- > 0; ) { On 2012/07/19 21:46:03, scherkus wrote: > nit: let's try to keep conditions as they are and put the iterator modifying > stuff in the third clause > > for (i = len - 1; i >= 0; --i) { > // ... > } Ok, I guess we can use a signed iterator since len is anyways <= 8. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.h (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.h:36: WebmEncoder(const FilePath& output_path, unsigned bitrate, bool realtime); On 2012/07/19 21:46:03, scherkus wrote: > you can use int for bitrate -- we do elsewhere in the media code Done. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.h:85: }; On 2012/07/19 21:46:03, scherkus wrote: > DISALLOW etc... Done.
lgtm with a better handling for file write operations. http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:255: if (fwrite(buffer, 1, len, output_) != len) Please flip error bit on WebmEncoder instance if fwrite / fseek fails for some reason.
http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:18: #include "third_party/libvpx/source/libvpx/libmkv/EbmlIDs.h" On 2012/07/20 11:10:36, Ivan Korotkov wrote: > On 2012/07/19 21:46:03, scherkus wrote: > > what's going on here? > > > > is there a particular header that causes grief? > > One of functions in EbmlWriter.h has argument named "private" :) Are we planning on fixing it upstream? http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:65: DCHECK(!sprite.isNull() && !sprite.empty()); nit: separate the DCHECKs http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:75: size_t frame_count = sprite.height() / width_; // Sprite is tiled vertically. do we need to do any alignment / padding for our yuv data? for reference see what we do in VideoFrame::AllocateYUV() http://src.chromium.org/viewvc/chrome/trunk/src/media/base/video_frame.cc?vie... http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:117: src, src_frame_size, sanity check: does libyuv have any assumptions on alignment or padding for input + output? http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:151: StartSubElement(EBML); { we have a style guide for a reason and I don't find the if/do-while appearance to add that much so I don't see why we'd warrant the exception here the additional scope levels are fine but we should have { } by themselves on separate lines
http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:18: #include "third_party/libvpx/source/libvpx/libmkv/EbmlIDs.h" On 2012/07/20 19:20:51, scherkus wrote: > On 2012/07/20 11:10:36, Ivan Korotkov wrote: > > On 2012/07/19 21:46:03, scherkus wrote: > > > what's going on here? > > > > > > is there a particular header that causes grief? > > > > One of functions in EbmlWriter.h has argument named "private" :) > > Are we planning on fixing it upstream? Not sure. This code not used anywhere but in vpxenc console tool and isn't part of the libvpx interface. I've just exported it to Chrome because there is no other WebM encoder present in tree. http://codereview.chromium.org/10784037/diff/8004/media/webm/chromeos/webm_en... media/webm/chromeos/webm_encoder.cc:60: DCHECK(!sprite.isNull() && !sprite.empty()); On 2012/07/19 21:46:03, scherkus wrote: > nit: separate the DCHECKs Done. http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:65: DCHECK(!sprite.isNull() && !sprite.empty()); On 2012/07/20 19:20:51, scherkus wrote: > nit: separate the DCHECKs Done. http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:75: size_t frame_count = sprite.height() / width_; // Sprite is tiled vertically. On 2012/07/20 19:20:51, scherkus wrote: > do we need to do any alignment / padding for our yuv data? > > for reference see what we do in VideoFrame::AllocateYUV() > http://src.chromium.org/viewvc/chrome/trunk/src/media/base/video_frame.cc?vie... Right, libyuv states that 16-byte alignment is more efficient. I've switched from vpx_img_wrap to vpx_img_alloc which takes care about aligning now. http://codereview.chromium.org/10784037/diff/14008/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:151: StartSubElement(EBML); { On 2012/07/20 19:20:51, scherkus wrote: > we have a style guide for a reason and I don't find the if/do-while appearance > to add that much so I don't see why we'd warrant the exception here > > the additional scope levels are fine but we should have { } by themselves on > separate lines Done.
lgtm w/ nits http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/ebml_w... File media/webm/chromeos/ebml_writer.cc (right): http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/ebml_w... media/webm/chromeos/ebml_writer.cc:6: #include "media/webm/chromeos/ebml_writer.h" nit: header include order has the .h first followed by a blank line followed by rest of includes http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:90: DCHECK(codec_iface); should these be errors be handled? http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:92: DCHECK_EQ(VPX_CODEC_OK, ret); ditto? http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:108: if (VPX_CODEC_OK != ret) nit: flip comparison -- we don't compare against a constant on the LHS http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:138: if (VPX_CODEC_OK != ret) ditto
Added file error handling in EbmlSerialize/EbmlWrite as well. http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/ebml_w... File media/webm/chromeos/ebml_writer.cc (right): http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/ebml_w... media/webm/chromeos/ebml_writer.cc:6: #include "media/webm/chromeos/ebml_writer.h" On 2012/07/23 22:23:43, scherkus wrote: > nit: header include order has the .h first followed by a blank line followed by > rest of includes Done. http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... File media/webm/chromeos/webm_encoder.cc (right): http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:90: DCHECK(codec_iface); On 2012/07/23 22:23:43, scherkus wrote: > should these be errors be handled? These 2 shouldn't normally happen and denote a problem with the libvpx configuration itself. http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:108: if (VPX_CODEC_OK != ret) On 2012/07/23 22:23:43, scherkus wrote: > nit: flip comparison -- we don't compare against a constant on the LHS Done. http://codereview.chromium.org/10784037/diff/16003/media/webm/chromeos/webm_e... media/webm/chromeos/webm_encoder.cc:138: if (VPX_CODEC_OK != ret) On 2012/07/23 22:23:43, scherkus wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivankr@chromium.org/10784037/17004
Try job failure for 10784037-17004 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivankr@chromium.org/10784037/17004
Change committed as 148024 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
