Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
450693006:
VideoToolbox encoder for cast senders. (Closed)
|
Created:
6 years, 4 months ago by jfroy Modified:
6 years, 1 month ago CC:
imcheng+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, pwestin+watch_google.com, miu+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org, hubbe, DaleCurtis, sandersd (OOO until July 31), chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[cast] VideoToolbox encoder
VideoToolbox is the hardware encoding library on iOS and OS X. This patch adds
a new VideoEncoder to the cast implementation that will be used when the video
codec is H.264.
Cast supports "external encoders" via the VideoEncodeAccelerator interface. At
first glance it would appear to be a better fit than subclassing VideoEncoder.
However, VideoEncodeAccelerator is not a good fit for VideoToolbox,
particularly with how IO buffers are managed. Furthermore, such an
implementation would require a large portion of the content module, which is
largely unnecessary on iOS.
BUG=401627
Committed: https://crrev.com/3eb789bc38793698755e8e75d5850989696ff97d
Cr-Commit-Position: refs/heads/master@{#305141}
Patch Set 1 #
Total comments: 64
Patch Set 2 : Fix lint and formatting issues. #
Total comments: 10
Patch Set 3 : Remove pragma marks, debug logging. #
Total comments: 19
Patch Set 4 : Remove C++11 features, use proper DCHECK macros, remove __func__ from logs, other review issues. #Patch Set 5 : Address review feedback from miu. #Patch Set 6 : Remove scoped CF pointer typedefs, cleanup SetSessionProperty, fix compile errors. #
Total comments: 11
Patch Set 7 : Edit comments per review feedback and for clarity. #
Total comments: 16
Patch Set 8 : Address review feedback. #Patch Set 9 : Non-keyframes now use the previous frame as their referenced frame ID. #Patch Set 10 : Add comment explaining the opt-in CVPixelBuffer in a VideoFrame optimization. #Patch Set 11 : Build file and video_sender changes for the VideoToolbox encoder. #Patch Set 12 : Rebase #Patch Set 13 : Fix syntax error in cast.gyp. #Patch Set 14 : Update to use VideoToolbox glue code, support initialization result callback, build files, integrat… #Patch Set 15 : Rebase build files on top of media_for_cast_ios component defined in 581803003. #
Total comments: 20
Patch Set 16 : Rebase #Patch Set 17 : Address review issues. Tests are forthcoming. #Patch Set 18 : Modernize to C++11 per the rules at http://chromium-cpp.appspot.com/ #
Total comments: 1
Patch Set 19 : Rebase #Patch Set 20 : Rebase #Patch Set 21 : Rename capture_time to reference_time to track media/cast. #Patch Set 22 : Fix the encoder to work with the new cast reference time code. #Patch Set 23 : Basic unit tests for the encoder, a few more comments and color attachments on buffers wrapping Vid… #Patch Set 24 : Add the unit tests file to BUILD.gn. #Patch Set 25 : Fix minor lint issues. #Patch Set 26 : Let the tests fail gracefully if VT is not available. #Patch Set 27 : Need to bail out of SetUp if VideoToolbox is not available. #Patch Set 28 : Implemented video frame factory. Reworked unit tests to be independent. #Patch Set 29 : Fix unit test link error. #
Total comments: 12
Patch Set 30 : Remove useless include and clean up in unit tests. #Messages
Total messages: 53 (4 generated)
https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:121: #pragma mark - Chrome doesn't typically do this. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:125: std::ostream& operator<<(std::ostream& out, const CFStringRef& cfstring) { All these osteram functions are just to support DLOGs. You should just remove these and the excessive DLOGing. Same with the error message conversion functions above. Don't ship unnecessary code. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:212: bool SetSessionProperty(VTSessionRef session, Is this really templatized for just logging purposes? https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:276: Initialize(); You only call this here, so why is it a separate method? https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:280: Teardown(); Same.
https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:121: #pragma mark - On 2014/08/07 22:49:23, rsesek wrote: > Chrome doesn't typically do this. Acknowledged. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:125: std::ostream& operator<<(std::ostream& out, const CFStringRef& cfstring) { On 2014/08/07 22:49:22, rsesek wrote: > All these osteram functions are just to support DLOGs. You should just remove > these and the excessive DLOGing. Same with the error message conversion > functions above. Don't ship unnecessary code. Acknowledged. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:212: bool SetSessionProperty(VTSessionRef session, On 2014/08/07 22:49:22, rsesek wrote: > Is this really templatized for just logging purposes? Mostly yes, but it's also nice to have setters with native types that box then call the this function. If I were to remove the logging I could make it a normal function. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:276: Initialize(); On 2014/08/07 22:49:23, rsesek wrote: > You only call this here, so why is it a separate method? The current code doesn't do it, but I have a suspicion I might need to teardown and initialize compression sessions on demand (due to encoding parameter changes). I structured the code to make this easier to retrofit later. https://codereview.chromium.org/450693006/diff/20001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:280: Teardown(); On 2014/08/07 22:49:22, rsesek wrote: > Same. See my reply above.
https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:19: bool SetSessionProperty(VTSessionRef session, Mark this static, or remove static from all of these helpers and just put them in the anonymous namespace. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:73: encode_next_frame_as_keyframe_(false) { using_hardware_ is uninitialized and will remain uninitialized on OS X if VTSessionCopyProperty() doesn't return noErr. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:122: DLOG(ERROR) << __func__ << " VTCompressionSessionCreate failed: " << status; I'd remove __func__ from all your DLOGs. It should be clear based on the message what went wrong, and if not, the file-line information is already included in the output. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:182: if (compression_properties_) { This is always NULL. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:233: DictionaryPtr frame_props(nullptr); This will automatically initialize to NULL. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:320: DCHECK(media::VideoFrame::NumPlanes(format) <= MAX_PLANES); DCHECK_LE https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:446: static_assert(sizeof(NalSizeType) == 1 || sizeof(NalSizeType) == 2 || I believe this requires C++11 library support, which we cannot yet use. Use COMPILE_ASSERT in base, instead. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:454: DCHECK(bytes_left > sizeof(NalSizeType)); DCHECK_GT https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.h:9: #include <VideoToolbox/VideoToolbox.h> nit: blank line between C system headers and C++ system headers https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.h:22: typedef base::ScopedCFTypeRef<CVPixelBufferRef> PixelBufferPtr; Remove this typedef. It's not typical to do this for scoped types, since it hides the important ownership information. It's only typical to typedef if you're specifying a custom deleter traits, in which case Scoped should also be in the name. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.h:44: typedef base::ScopedCFTypeRef<CFDictionaryRef> DictionaryPtr; Similarly, remove.
Where are the BUILD file changes? More importantly, where are the unit/functional tests? First round of comments (started reviewing PS1 yesterday, not quite finished): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:19: static const char* GetCVErrorString(CVReturn error) { This seems overkill for debug logging. Suggest you remove it unless you feel the DLOG's would be useful as VLOG's, and even then, the numerical code is probably sufficient. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:56: static const char* GetVTErrorString(OSStatus error) { ditto: re: overkill for debug logging https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:125: std::ostream& operator<<(std::ostream& out, const CFStringRef& cfstring) { Rather than rolling your own, how about using the existing libraries? https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s... https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:149: std::ostream& operator<<(std::ostream& out, const CFNumberRef& cfnumber) { For this and the other ostream::operator<< definitions, perhaps the functions in src/base/mac/foundation_util.h can be used instead? If not, you should define these functions in foundation_util.h so they can be used by everyone. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:372: SetSessionProperty(session, key, CFTypeEmittable(value), value); Here and in ConfigureSession() below: If you're going to ingore the return value, why not just call VTSetSessionProperty directly? Said another way: Don't ignore the return values. If any calls are not OK, something should fail (e.g., all calls to EncodeVideoFrame should return false). Given this is being invoked from the ctor, perhaps the application should crash via CHECK_EQ(VTSetSessionProperty(...), OK)? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:422: // If the compression session exists, invalidate it. This blocks until all re: "This blocks" Never block the IO thread. How about using base::WeakPtr<> to cancel all outstanding callbacks? You can find examples of this throughout src/media/cast code. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:445: PixelBufferPtr pixel_buffer(video_frame->cv_pixel_buffer(), VideoFrame::cv_pixel_buffer() will return NULL no matter what. Did you mean to make a VideoFrame::SetCVPixelBuffer() call in WrapVideoFrame()? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:456: if (capture_time.is_null()) { It's an illegal argument for capture_time to be null. You could replace this if-statement with: DCHECK(!capture_time.is_null()); ...and put this at the top of this method (on line 438). https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:459: timestamp_cm = CMTimeMake(capture_time.ToInternalValue(), USEC_PER_SEC); Never use ToInternalValue(). The timestamps are relative to some starting point when being converted to intervals. You could, conventionally, make the first frame be t=0. For example, the code in this else clause could become: if (first_frame_capture_time_.is_null()) first_frame_capture_time_ = capture_time; timestamp_cm = CMTimeMake((capture_time - first_frame_capture_time_).InMicroseconds(), USEC_PER_SEC); https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:463: FrameContext* fc = new FrameContext(); coding style: Don't abbreviate variable names. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:463: FrameContext* fc = new FrameContext(); For safety, use scoped_ptr<> for this, and call scoped_ptr<>::release() when transferring ownership to VTCompressionSessionEncodeFrame(). https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:504: if (!compression_session_) { Should this be DCHECK(compression_session_) instead? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:509: // if (new_bit_rate) { Why is this commented out? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:523: void H264VideoToolboxEncoder::LatestFrameIdToReference(uint32 /*frame_id*/) {} For documentation, can you make the body of this method the following: // Do nothing; not supported. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:537: H264VideoToolboxEncoder::WrapVideoFrame(media::VideoFrame& frame) { nit: indent 4 spaces https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:571: DCHECK(media::VideoFrame::NumPlanes(format) <= MAX_PLANES); Consider moving this DCHECK up to line 542, so the error is caught sooner. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:588: calloc(1, nit: malloc(std::max(...)); https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:598: &descriptor, I think you meant descriptor and not &descriptor here. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:598: &descriptor, It's weird that you're passing an uninitialized structure here. The documentation for CVPixelBufferCreateWithPlanarBytes() says this can be NULL. Are you absolutely sure the release callback won't be invoked if you provide NULL here? That would seem to point to a significant bug in the library. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:606: &frame, You meant frame.get(), not &frame. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:633: scoped_ptr<FrameContext> fc(reinterpret_cast<FrameContext*>(frame_opaque)); style: Don't abbreviating variable names. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:650: if (sample_count > 1) { Is this expected? Perhaps this should be DCHECK_EQ(sample_count, 1); https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:675: CMSampleTimingInfo timing_info; Should you be mapping timing_info back into a capture_time? Could the codec do temporal interpolation of frames? If not, then please wrap these lines of code since they should only be executed for debug logging: if (DVLOG_IS_ON(3)) { ... } https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:694: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; This needs to be true to the H264 codec. If you don't know what the exact frame dependencies are, you can always play it safe and do this (like in external_video_encoder.cc): encoded_frame->referenced_frame_id = encoded_frame->frame_id - 1; This means a cast sender or receiver is not allowed to drop any non-key frames (i.e., all frames are dependent on their immediately prior frame, except for key frames). I believe that's exactly the case for the H264 codec. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:712: std::string& annexb_buffer) { Chromium style prohibits passing by reference. Pass by pointer instead. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:713: static_assert(sizeof(NalSizeType) == 1 || Replace static_assert with COMPILE_ASSERT for Chromium code. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:723: bytes_left -= sizeof(NalSizeType); Consider using base::BigEndianReader instead. It would simplify this code, in that you wouldn't have to keep track of bytes_left and the avcc_buffer pointer. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:736: std::string& annexb_buffer, ditto: Pass by pointer, not by reference. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:26: explicit H264VideoToolboxEncoder( Don't need 'explicit' with 2-arg ctor. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:76: scoped_refptr<CastEnvironment> cast_environment_; nit: const scoped_refptr<CastEnvironment> cast_environment_; ^^^^ (since this member is never changed after construction) https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:102: bool using_hardware_; It doesn't look like this is being used for anything. It's being set, but did you mean to expose it externally for some reason? If you decide to keep it, please add using_hardware_(false) to the ctor's initializer list.
https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:19: bool SetSessionProperty(VTSessionRef session, On 2014/08/08 17:24:16, rsesek wrote: > Mark this static, or remove static from all of these helpers and just put them > in the anonymous namespace. Oversight from removing the template parameter. Thanks. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:73: encode_next_frame_as_keyframe_(false) { On 2014/08/08 17:24:17, rsesek wrote: > using_hardware_ is uninitialized and will remain uninitialized on OS X if > VTSessionCopyProperty() doesn't return noErr. I'm just going to remove that member, it is not used in the current implementation. If it ever becomes useful to know this, I'll add it back. As a note, certain properties can never fail to be fetched, such as this one, provided the session is valid. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:122: DLOG(ERROR) << __func__ << " VTCompressionSessionCreate failed: " << status; On 2014/08/08 17:24:17, rsesek wrote: > I'd remove __func__ from all your DLOGs. It should be clear based on the message > what went wrong, and if not, the file-line information is already included in > the output. Acknowledged. It was largely inspired by the logging statements in most of the external encoders / decoders (content/common/gpu/media). https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:182: if (compression_properties_) { On 2014/08/08 17:24:16, rsesek wrote: > This is always NULL. A prior prototype implementation used this additional properties dictionary while the current does not. I will remove this member entirely. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:233: DictionaryPtr frame_props(nullptr); On 2014/08/08 17:24:17, rsesek wrote: > This will automatically initialize to NULL. Acknowledged. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:320: DCHECK(media::VideoFrame::NumPlanes(format) <= MAX_PLANES); On 2014/08/08 17:24:17, rsesek wrote: > DCHECK_LE Acknowledged. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:446: static_assert(sizeof(NalSizeType) == 1 || sizeof(NalSizeType) == 2 || On 2014/08/08 17:24:17, rsesek wrote: > I believe this requires C++11 library support, which we cannot yet use. Use > COMPILE_ASSERT in base, instead. Acknowledged. https://codereview.chromium.org/450693006/diff/40001/media/cast/sender/h264_v... media/cast/sender/h264_vt_encoder.cc:454: DCHECK(bytes_left > sizeof(NalSizeType)); On 2014/08/08 17:24:17, rsesek wrote: > DCHECK_GT Acknowledged.
https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:19: static const char* GetCVErrorString(CVReturn error) { On 2014/08/08 17:53:06, miu wrote: > This seems overkill for debug logging. Suggest you remove it unless you feel > the DLOG's would be useful as VLOG's, and even then, the numerical code is > probably sufficient. This has all been removed in a following PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:56: static const char* GetVTErrorString(OSStatus error) { On 2014/08/08 17:53:07, miu wrote: > ditto: re: overkill for debug logging This has all been removed in a following PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:125: std::ostream& operator<<(std::ostream& out, const CFStringRef& cfstring) { On 2014/08/08 17:53:07, miu wrote: > Rather than rolling your own, how about using the existing libraries? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s... I had no idea those existed. This has all been removed in a following PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:149: std::ostream& operator<<(std::ostream& out, const CFNumberRef& cfnumber) { On 2014/08/08 17:53:08, miu wrote: > For this and the other ostream::operator<< definitions, perhaps the functions in > src/base/mac/foundation_util.h can be used instead? > > If not, you should define these functions in foundation_util.h so they can be > used by everyone. This has all been removed in a following PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:372: SetSessionProperty(session, key, CFTypeEmittable(value), value); On 2014/08/08 17:53:08, miu wrote: > Here and in ConfigureSession() below: > > If you're going to ingore the return value, why not just call > VTSetSessionProperty directly? > > Said another way: Don't ignore the return values. If any calls are not OK, > something should fail (e.g., all calls to EncodeVideoFrame should return false). > Given this is being invoked from the ctor, perhaps the application should crash > via CHECK_EQ(VTSetSessionProperty(...), OK)? The final SetSessionProperty used to log the error, but in a later PS (per reviewer request) I removed almost all debug logging. Failing on property errors is probably not a good idea. Some encoders will reject certain properties or property values for whatever reason. It is also hardware-dependent. Some properties are rejected at certain times but not others (in-flight frames or not, state of the encoder, power state, battery state, etc). I consider the properties I am setting as best-efforts, meaning that failure is tolerated. I'm gathering data on the implementation and perhaps certain properties should indeed lead to encoding failure or initialization failure if they can't be set. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:422: // If the compression session exists, invalidate it. This blocks until all On 2014/08/08 17:53:07, miu wrote: > re: "This blocks" > > Never block the IO thread. How about using base::WeakPtr<> to cancel all > outstanding callbacks? You can find examples of this throughout src/media/cast > code. This will run in the video thread presumably. There is no way to avoid the thread join done by VideoToolbox. Weak pointers are untenable because VideoToolbox is a dumb C API; it only accepts a void* opaque context parameter. The only race-free design is this: block until invalidation is complete and be guaranteed that the output callback will not occur again. If encoders are destroyed on the IO thread, this is more problematic, although arguably at tear down there should not be any inbound or outbound IO left. In that case I would probably wrap the VideoToolbox encoder in an object and use weak pointers back to the main encoder object, much like the external encoder implementation. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:445: PixelBufferPtr pixel_buffer(video_frame->cv_pixel_buffer(), On 2014/08/08 17:53:07, miu wrote: > VideoFrame::cv_pixel_buffer() will return NULL no matter what. Did you mean to > make a VideoFrame::SetCVPixelBuffer() call in WrapVideoFrame()? No. The idea here is that the frame producer will create VideoFrame objects wrapping a CVPixelBuffer backed by the encoder's CVPixelBufferPool, which it will then submit to the cast sender's video input object. We can discuss the details in chat. For tab casting, it is true that the video frames will not wrap a CVPixelBuffer (although that optimization could be made). https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:456: if (capture_time.is_null()) { On 2014/08/08 17:53:09, miu wrote: > It's an illegal argument for capture_time to be null. You could replace this > if-statement with: > > DCHECK(!capture_time.is_null()); > > ...and put this at the top of this method (on line 438). Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:459: timestamp_cm = CMTimeMake(capture_time.ToInternalValue(), USEC_PER_SEC); On 2014/08/08 17:53:06, miu wrote: > Never use ToInternalValue(). The timestamps are relative to some starting point > when being converted to intervals. You could, conventionally, make the first > frame be t=0. For example, the code in this else clause could become: > > if (first_frame_capture_time_.is_null()) > first_frame_capture_time_ = capture_time; > timestamp_cm = CMTimeMake((capture_time - > first_frame_capture_time_).InMicroseconds(), USEC_PER_SEC); CMTime is also relative to a clock (which is not part of the data structure either, you need to have implicit knowledge of the clock of a CMTime). This code merely translates one clock-relative datatype to another (CMTime is effectively a rational number with flags). Therefore I think using ToInternalValue is exactly the right approach. Also note that the EncodedFrame emitted for this input frame will use the capture_time set in the frame context (see below). https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:463: FrameContext* fc = new FrameContext(); On 2014/08/08 17:53:07, miu wrote: > coding style: Don't abbreviate variable names. Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:463: FrameContext* fc = new FrameContext(); On 2014/08/08 17:53:08, miu wrote: > For safety, use scoped_ptr<> for this, and call scoped_ptr<>::release() when > transferring ownership to VTCompressionSessionEncodeFrame(). Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:504: if (!compression_session_) { On 2014/08/08 17:53:08, miu wrote: > Should this be DCHECK(compression_session_) instead? I used a log here because I was worried that since initialization is assumed to succeed by VideoSender (it has no way of asking or checking if an encoder actually initialized property and blindly reports success via the status callback) that this may be called with an invalid session. In the latest PS, this method is empty with a NOTE explaining that VideoToolbox seems to ignore bitrate change requests once the encoder is active. I need to research its behavior more (which is made more complicated by the facts that it varies based on the underlying hardware and that iOS 8 is beta and has bugs). https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:509: // if (new_bit_rate) { On 2014/08/08 17:53:07, miu wrote: > Why is this commented out? See my comment above. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:523: void H264VideoToolboxEncoder::LatestFrameIdToReference(uint32 /*frame_id*/) {} On 2014/08/08 17:53:08, miu wrote: > For documentation, can you make the body of this method the following: > > // Do nothing; not supported. Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:537: H264VideoToolboxEncoder::WrapVideoFrame(media::VideoFrame& frame) { On 2014/08/08 17:53:08, miu wrote: > nit: indent 4 spaces I've been formatting everything via clang-format / git cl format. The latest PS is a bit different than what is here. What should be indented here? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:571: DCHECK(media::VideoFrame::NumPlanes(format) <= MAX_PLANES); On 2014/08/08 17:53:07, miu wrote: > Consider moving this DCHECK up to line 542, so the error is caught sooner. Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:588: calloc(1, On 2014/08/08 17:53:08, miu wrote: > nit: malloc(std::max(...)); The memory needs to be zeroed. I thought calloc was the appropriate idiom to use in that case. Should I memset after instead? https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:598: &descriptor, On 2014/08/08 17:53:07, miu wrote: > I think you meant descriptor and not &descriptor here. Good catch. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:598: &descriptor, On 2014/08/08 17:53:08, miu wrote: > It's weird that you're passing an uninitialized structure here. The > documentation for CVPixelBufferCreateWithPlanarBytes() says this can be NULL. > > Are you absolutely sure the release callback won't be invoked if you provide > NULL here? That would seem to point to a significant bug in the library. Yes, I've extensively tested this behavior, decompiled CoreVideo and confirmed it with other developers. CV completely ignores the plane pointers when determining whether or not to call its release callback, but it does use the plane pointers for data. I've filed a bug with Apple over this. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:606: &frame, On 2014/08/08 17:53:09, miu wrote: > You meant frame.get(), not &frame. No, this is correct, since frame here is a media::VideoFrame&. But linting pointed out that non-const references are not allowed, so I changed the function to use a scoped_refptr and adjusted the code, all in a later PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:633: scoped_ptr<FrameContext> fc(reinterpret_cast<FrameContext*>(frame_opaque)); On 2014/08/08 17:53:06, miu wrote: > style: Don't abbreviating variable names. Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:650: if (sample_count > 1) { On 2014/08/08 17:53:07, miu wrote: > Is this expected? Perhaps this should be DCHECK_EQ(sample_count, 1); For video, not likely (it certainly is for audio). I'm not sure what our best option is here. Future hardware could behavior differently and I'd like to know when that happens and on crash the client. A debug log or assertion won't tell us what happens in the wild. I think an assertion is better for now though. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:675: CMSampleTimingInfo timing_info; On 2014/08/08 17:53:09, miu wrote: > Should you be mapping timing_info back into a capture_time? Could the codec do > temporal interpolation of frames? > > If not, then please wrap these lines of code since they should only be executed > for debug logging: > > if (DVLOG_IS_ON(3)) { > ... > } Didn't know you could do that. The timing info was only for debug logging and that code is removed in a later PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:694: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; On 2014/08/08 17:53:07, miu wrote: > This needs to be true to the H264 codec. If you don't know what the exact frame > dependencies are, you can always play it safe and do this (like in > external_video_encoder.cc): > > encoded_frame->referenced_frame_id = encoded_frame->frame_id - 1; > > This means a cast sender or receiver is not allowed to drop any non-key frames > (i.e., all frames are dependent on their immediately prior frame, except for key > frames). I believe that's exactly the case for the H264 codec. H.264 is allowed multiple frame references backward and forward. I do advise the encoder not to use forward references (i.e. no frame reordering), but it could totally decide to ignore that (unless the profile and forbids it, which it does not as the code stands). I think forcing the previous frame as a reference is too pessimistic. VideoToolbox will emit keyframes are regular internals. Those absolutely cannot be dropped because parameter sets are prefixed on those frames and required for any subsequent frames until the next keyframe. But anything in the middle I think is fair game to be dropped. That may mean some corrupt or dropped (at display / decode time) frames on the receiver. But we'll save a lot of unnecessary retransmissions. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:712: std::string& annexb_buffer) { On 2014/08/08 17:53:07, miu wrote: > Chromium style prohibits passing by reference. Pass by pointer instead. Yep linting indicated that. Fixed in a later PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:713: static_assert(sizeof(NalSizeType) == 1 || On 2014/08/08 17:53:07, miu wrote: > Replace static_assert with COMPILE_ASSERT for Chromium code. Yep, other reviewer pointed it out. Fixed in a later PS. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:723: bytes_left -= sizeof(NalSizeType); On 2014/08/08 17:53:08, miu wrote: > Consider using base::BigEndianReader instead. It would simplify this code, in > that you wouldn't have to keep track of bytes_left and the avcc_buffer pointer. I can't because BigEndianReader does not have a templated read method. I'd have to branch on the sizes. I made this a template function to get as much performance as possible for the AVCC->Annex B conversion. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:736: std::string& annexb_buffer, On 2014/08/08 17:53:08, miu wrote: > ditto: Pass by pointer, not by reference. Yep, fixed in a later PS.
On 2014/08/08 17:53:09, miu wrote: > Where are the BUILD file changes? More importantly, where are the > unit/functional tests? I was asked to provide justification for my other diff extended VideoFrame to carry CVPixelBuffer objects, so I posted this. It obviously needs tests, and it needs integration code. Those will follow. I'm glad to discuss and improve this code specifically in the mean time.
https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:26: explicit H264VideoToolboxEncoder( On 2014/08/08 17:53:09, miu wrote: > Don't need 'explicit' with 2-arg ctor. Acknowledged. https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:76: scoped_refptr<CastEnvironment> cast_environment_; On 2014/08/08 17:53:09, miu wrote: > nit: const scoped_refptr<CastEnvironment> cast_environment_; > ^^^^ > > (since this member is never changed after construction) Acknowledged.
https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.h:102: bool using_hardware_; On 2014/08/08 17:53:09, miu wrote: > It doesn't look like this is being used for anything. It's being set, but did > you mean to expose it externally for some reason? > > If you decide to keep it, please add using_hardware_(false) to the ctor's > initializer list. Acknowledged.
Mac-wise, this is in pretty good shape now. I'm not too familiar with the media-related aspects, though. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:23: CFNumberCreate(NULL, kCFNumberSInt32Type, &value)); You're converting a uint32 to an int32. That seems dangerous. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:224: // NOTE: VideoToolbox does not seem to support bitrate reconfiguration. Remove "NOTE:" since this is a comment. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:235: // NOTE: Not supported by VideoToolbox in any meaningful manner. Same. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:19: // VideoToolbox implementation of the media::cast::VideoEncoder interface. Hoist the comment about thread safety up to this class-level comment. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:75: // binding occurs in Initialize(), not the ctor. This comment is misleading, since Initialize is only called from the ctor.
https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:23: CFNumberCreate(NULL, kCFNumberSInt32Type, &value)); On 2014/08/12 00:06:12, rsesek wrote: > You're converting a uint32 to an int32. That seems dangerous. There are no constants for unsigned types in CFNumber (https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc...). Presumably CFNumber copies the by-pointer value bit for bit without interpretation. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:224: // NOTE: VideoToolbox does not seem to support bitrate reconfiguration. On 2014/08/12 00:06:12, rsesek wrote: > Remove "NOTE:" since this is a comment. Acknowledged. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:235: // NOTE: Not supported by VideoToolbox in any meaningful manner. On 2014/08/12 00:06:12, rsesek wrote: > Same. Acknowledged. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:19: // VideoToolbox implementation of the media::cast::VideoEncoder interface. On 2014/08/12 00:06:12, rsesek wrote: > Hoist the comment about thread safety up to this class-level comment. Acknowledged. https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:75: // binding occurs in Initialize(), not the ctor. On 2014/08/12 00:06:12, rsesek wrote: > This comment is misleading, since Initialize is only called from the ctor. Acknowledged.
On 2014/08/12 00:06:12, rsesek wrote: > Mac-wise, this is in pretty good shape now. I'm not too familiar with the > media-related aspects, though. I'm still working on integration. It's challenging on the iOS front because of build issues (missing assembly for arm64, other issues). The cast implementation relies on a very small subset of media (mostly VideoFrame and AudioBus). Thanks a lot for your feedback. It is tremendously appreciated. And your patience in dealing with a noob.
On 2014/08/12 01:08:36, jfroy wrote: > Thanks a lot for your feedback. It is tremendously appreciated. And your > patience in dealing with a noob. BTW--I haven't forgotten about this. Things are a bit busy with the upcoming M38 branch. I'll try to review again tomorrow.
No worries I have plenty of work to do! On Mon, Aug 11, 2014 at 20:45 <miu@chromium.org> wrote: > On 2014/08/12 01:08:36, jfroy wrote: > > Thanks a lot for your feedback. It is tremendously appreciated. And your > > patience in dealing with a noob. > > BTW--I haven't forgotten about this. Things are a bit busy with the > upcoming > M38 branch. I'll try to review again tomorrow. > > > https://codereview.chromium.org/450693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:23: CFNumberCreate(NULL, kCFNumberSInt32Type, &value)); On 2014/08/12 01:04:15, jfroy wrote: > On 2014/08/12 00:06:12, rsesek wrote: > > You're converting a uint32 to an int32. That seems dangerous. > > There are no constants for unsigned types in CFNumber > (https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc...). > Presumably CFNumber copies the by-pointer value bit for bit without > interpretation. Right, so either use kCFNumberSInt64Type or base/numerics/safe_conversions.h.
On 2014/08/12 18:47:09, rsesek wrote: > https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... > File media/cast/sender/h264_vt_encoder.cc (right): > > https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... > media/cast/sender/h264_vt_encoder.cc:23: CFNumberCreate(NULL, > kCFNumberSInt32Type, &value)); > On 2014/08/12 01:04:15, jfroy wrote: > > On 2014/08/12 00:06:12, rsesek wrote: > > > You're converting a uint32 to an int32. That seems dangerous. > > > > There are no constants for unsigned types in CFNumber > > > (https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc...). > > Presumably CFNumber copies the by-pointer value bit for bit without > > interpretation. > > Right, so either use kCFNumberSInt64Type or base/numerics/safe_conversions.h. Semantically this is a reinterpret_cast. There will be no truncation or loss as long as the bit size matches. Reading a value out of a CFNumber is done with a void* out parameter as well, which is another bit-for-bit copy. I don't see the need for expanding the internal storage, or for using a safe value cast (which I agree would be warranted if casting from an unsigned- to a signed-typed variable).
On 2014/08/12 19:40:50, jfroy wrote: > On 2014/08/12 18:47:09, rsesek wrote: > > > https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... > > File media/cast/sender/h264_vt_encoder.cc (right): > > > > > https://codereview.chromium.org/450693006/diff/100001/media/cast/sender/h264_... > > media/cast/sender/h264_vt_encoder.cc:23: CFNumberCreate(NULL, > > kCFNumberSInt32Type, &value)); > > On 2014/08/12 01:04:15, jfroy wrote: > > > On 2014/08/12 00:06:12, rsesek wrote: > > > > You're converting a uint32 to an int32. That seems dangerous. > > > > > > There are no constants for unsigned types in CFNumber > > > > > > (https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc...). > > > Presumably CFNumber copies the by-pointer value bit for bit without > > > interpretation. > > > > Right, so either use kCFNumberSInt64Type or base/numerics/safe_conversions.h. > > Semantically this is a reinterpret_cast. There will be no truncation or loss as > long as the bit size matches. Reading a value out of a CFNumber is done with a > void* out parameter as well, which is another bit-for-bit copy. I don't see the > need for expanding the internal storage, or for using a safe value cast (which I > agree would be warranted if casting from an unsigned- to a signed-typed > variable). Fair enough. LGTM
Waiting for review by miu.
On 2014/08/15 22:11:37, jfroy wrote: > Waiting for review by miu. Gettin' to it today. Thanks for being patient. ;-)
No rush miu, I'm on vacation this week. On Mon, Aug 18, 2014 at 18:06 <miu@chromium.org> wrote: > On 2014/08/15 22:11:37, jfroy wrote: > > Waiting for review by miu. > > Gettin' to it today. Thanks for being patient. ;-) > > https://codereview.chromium.org/450693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:459: timestamp_cm = CMTimeMake(capture_time.ToInternalValue(), USEC_PER_SEC); On 2014/08/08 23:23:26, jfroy wrote: > On 2014/08/08 17:53:06, miu wrote: > > Never use ToInternalValue(). The timestamps are relative to some starting > point My point here is that ToInternalValue() is explicitly described as "do not use in code." It only has semantics internally to TimeTicks. If the time origin does not matter, how about: ... = CMTimeMake((capture_time - base::TimeTicks()).InMicroseconds(), USEC_PER_SEC); https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:8: #include <vector> Looks like std::vector isn't being used anywhere. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( IMHO, it would be cleaner for the CVPixelBuffer to be owned by media::VideoFrame. VideoFrame could create the CVPixelBuffer lazily, and keep it around until the VideoFrame dies. This would allow the same CVPixelBuffer to be re-used rather than having to create a new one for each use. In other words, this should be a method of VideoFrame. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:395: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; If this is true, the encoder might not be very space-efficient since it would have to: 1) encode key frames more often; and/or 2) encode larger deltas. You may want to take a look at what the Android folks are doing for their H264 encoder. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:73: const VideoSenderConfig cast_config_; nit: You don't need this member, as it's only used in Initialize() and ConfigureSession(), both of which are called from the ctor. So, you could just pass it as an arg when calling these two methods.
https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:8: #include <vector> On 2014/08/25 19:21:17, miu wrote: > Looks like std::vector isn't being used anywhere. Done. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( On 2014/08/25 19:21:17, miu wrote: > IMHO, it would be cleaner for the CVPixelBuffer to be owned by > media::VideoFrame. VideoFrame could create the CVPixelBuffer lazily, and keep > it around until the VideoFrame dies. This would allow the same CVPixelBuffer to > be re-used rather than having to create a new one for each use. > > In other words, this should be a method of VideoFrame. I really don't want to burden VideoFrame with more platform-specific code (this sentiment has been expressed by others as well reviewing the other patch). In addition, the CVPixelBuffer must own the VideoFrame in this situation because VideoToolbox manages the lifetime of CVPixelBuffers, not VideoFrames. The encoder may hold on to input frames for some time, and the encoding process is asynchronous. It is very likely that the VideoFrame would be destroyed before the CVPixelBuffer, if the pixel buffer did not take a reference on the VideoFrame. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:395: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; On 2014/08/25 19:21:17, miu wrote: > If this is true, the encoder might not be very space-efficient since it would > have to: 1) encode key frames more often; and/or 2) encode larger deltas. You > may want to take a look at what the Android folks are doing for their H264 > encoder. In my experimentation (and there is no documentation for this) is that VideoToolbox does not report to the client the exact frame dependencies it used. Furthermore, if a frame depends on more than one frame, then a single "referenced frame" field is not going to be sufficient. As far as I understand it, this field is used for frame retransmission, so using the last encoded keyframe is a reasonable compromise. It will allow the stream to recover reasonably quickly from a burst of lost packets, which I understand is the typical scenario in a busy WiFi environment. The rate at which keyframes are encoded is under our control (see ConfigureSession), currently set at every 240 frames or 240 seconds. I admit I have done no performance analysis for these rates yet. I'll try to get in touch with the Android team to see if they have any (I don't have access to their source). I also don't know to what extent the hardware encoder respects these parameters. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:73: const VideoSenderConfig cast_config_; On 2014/08/25 19:21:17, miu wrote: > nit: You don't need this member, as it's only used in Initialize() and > ConfigureSession(), both of which are called from the ctor. So, you could just > pass it as an arg when calling these two methods. Done.
https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/1/media/cast/sender/h264_vt_en... media/cast/sender/h264_vt_encoder.cc:459: timestamp_cm = CMTimeMake(capture_time.ToInternalValue(), USEC_PER_SEC); On 2014/08/25 19:21:17, miu wrote: > On 2014/08/08 23:23:26, jfroy wrote: > > On 2014/08/08 17:53:06, miu wrote: > > > Never use ToInternalValue(). The timestamps are relative to some starting > > point > > My point here is that ToInternalValue() is explicitly described as "do not use > in code." It only has semantics internally to TimeTicks. If the time origin > does not matter, how about: > > ... = CMTimeMake((capture_time - base::TimeTicks()).InMicroseconds(), > USEC_PER_SEC); > Done.
https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( On 2014/08/25 20:59:13, jfroy wrote: > On 2014/08/25 19:21:17, miu wrote: > > IMHO, it would be cleaner for the CVPixelBuffer to be owned by > > media::VideoFrame. VideoFrame could create the CVPixelBuffer lazily, and keep > > it around until the VideoFrame dies. This would allow the same CVPixelBuffer > to > > be re-used rather than having to create a new one for each use. > > > > In other words, this should be a method of VideoFrame. > > I really don't want to burden VideoFrame with more platform-specific code (this > sentiment has been expressed by others as well reviewing the other patch). > > In addition, the CVPixelBuffer must own the VideoFrame in this situation because > VideoToolbox manages the lifetime of CVPixelBuffers, not VideoFrames. The > encoder may hold on to input frames for some time, and the encoding process is > asynchronous. It is very likely that the VideoFrame would be destroyed before > the CVPixelBuffer, if the pixel buffer did not take a reference on the > VideoFrame. Okay. I'm fine with this scheme. But, I don't think you should check in the other change (https://codereview.chromium.org/446163003/) which does the wrapping in the other order (VideoFrame wrapping CVPixelBuffer). https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:395: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; On 2014/08/25 20:59:13, jfroy wrote: > In my experimentation (and there is no documentation for this) is that > VideoToolbox does not report to the client the exact frame dependencies it used. > Furthermore, if a frame depends on more than one frame, then a single > "referenced frame" field is not going to be sufficient. Can it be determined from the encoded data? (read below) > As far as I understand it, this field is used for frame retransmission, so using > the last encoded keyframe is a reasonable compromise. It will allow the stream > to recover reasonably quickly from a burst of lost packets, which I understand > is the typical scenario in a busy WiFi environment. No, that's not at all the case. These are true frame dependencies, where the system must absolutely have frame X in order to make frame Y decodable. IIRC, the Android folks at one time always made frame X depend on frame X-1 (except for key frames). Not sure whether they ever resolved that. If there's no way to determine the true frame dependencies, you'll have no choice but to have every non-key frame depend on its immediately prior frame; or else bad things will happen.
https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( On 2014/08/25 21:47:40, miu wrote: > On 2014/08/25 20:59:13, jfroy wrote: > > On 2014/08/25 19:21:17, miu wrote: > > > IMHO, it would be cleaner for the CVPixelBuffer to be owned by > > > media::VideoFrame. VideoFrame could create the CVPixelBuffer lazily, and > keep > > > it around until the VideoFrame dies. This would allow the same > CVPixelBuffer > > to > > > be re-used rather than having to create a new one for each use. > > > > > > In other words, this should be a method of VideoFrame. > > > > I really don't want to burden VideoFrame with more platform-specific code > (this > > sentiment has been expressed by others as well reviewing the other patch). > > > > In addition, the CVPixelBuffer must own the VideoFrame in this situation > because > > VideoToolbox manages the lifetime of CVPixelBuffers, not VideoFrames. The > > encoder may hold on to input frames for some time, and the encoding process is > > asynchronous. It is very likely that the VideoFrame would be destroyed before > > the CVPixelBuffer, if the pixel buffer did not take a reference on the > > VideoFrame. > > Okay. I'm fine with this scheme. But, I don't think you should check in the > other change (https://codereview.chromium.org/446163003/) which does the > wrapping in the other order (VideoFrame wrapping CVPixelBuffer). This is essentially a fallback function. It is expected that in the final production version, this code path will not execute because the VideoFrames will wrap a CVPixelBuffer backed by the encoder's CVPixelBufferPool. It is there to ensure the encoder will not completely fail if the optimization enabled by 450693006 is not used by some client (perhaps Chrome for tab casting). https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:395: encoded_frame->referenced_frame_id = encoder->last_keyframe_id_; On 2014/08/25 21:47:40, miu wrote: > On 2014/08/25 20:59:13, jfroy wrote: > > In my experimentation (and there is no documentation for this) is that > > VideoToolbox does not report to the client the exact frame dependencies it > used. > > Furthermore, if a frame depends on more than one frame, then a single > > "referenced frame" field is not going to be sufficient. > > Can it be determined from the encoded data? (read below) I'd rather not have to decode the video layer data and put a ton of H.264 decoding code here or elsewhere, so conservatively no. > > > As far as I understand it, this field is used for frame retransmission, so > using > > the last encoded keyframe is a reasonable compromise. It will allow the stream > > to recover reasonably quickly from a burst of lost packets, which I understand > > is the typical scenario in a busy WiFi environment. > > No, that's not at all the case. These are true frame dependencies, where the > system must absolutely have frame X in order to make frame Y decodable. IIRC, > the Android folks at one time always made frame X depend on frame X-1 (except > for key frames). Not sure whether they ever resolved that. > > If there's no way to determine the true frame dependencies, you'll have no > choice but to have every non-key frame depend on its immediately prior frame; or > else bad things will happen. OK, I'll follow what the Android implementation did. I'm getting in touch with them regarding this specific issue (and other details).
Okay, this looks great now. Can you add the BUILD files changes and the code (in VideoSender?) that instantiates and uses your new encoder? https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:181: pixel_buffer = WrapVideoFrame(video_frame); Just to retain the findings of our discussion (what was primarily my confusion), can you add a comment here like: "VideoFrame was not generated from the accelerated capture code path that provides a CVPixelBuffer, so create an adapter." https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( On 2014/08/25 21:57:56, jfroy wrote: > This is essentially a fallback function. It is expected that in the final > production version, this code path will not execute because the VideoFrames will > wrap a CVPixelBuffer backed by the encoder's CVPixelBufferPool. It is there to > ensure the encoder will not completely fail if the optimization enabled by > 450693006 is not used by some client (perhaps Chrome for tab casting). I understand now. Thanks for being patient.
Working on the build file changes now. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:181: pixel_buffer = WrapVideoFrame(video_frame); On 2014/08/25 22:22:09, miu wrote: > Just to retain the findings of our discussion (what was primarily my confusion), > can you add a comment here like: "VideoFrame was not generated from the > accelerated capture code path that provides a CVPixelBuffer, so create an > adapter." Done. https://codereview.chromium.org/450693006/diff/120001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:247: base::ScopedCFTypeRef<CVPixelBufferRef> H264VideoToolboxEncoder::WrapVideoFrame( On 2014/08/25 22:22:09, miu wrote: > On 2014/08/25 21:57:56, jfroy wrote: > > This is essentially a fallback function. It is expected that in the final > > production version, this code path will not execute because the VideoFrames > will > > wrap a CVPixelBuffer backed by the encoder's CVPixelBufferPool. It is there to > > ensure the encoder will not completely fail if the optimization enabled by > > 450693006 is not used by some client (perhaps Chrome for tab casting). > > I understand now. Thanks for being patient. No problem :)
Working on stubs for CoreVideo and VideoToolbox, since the code only supports very recent versions of OS X and iOS. It's not even enough to rely on weak linking, because entire frameworks are missing from the SDKs being used to build.
Still working on media framework glue code.
I've uploaded a big patch set which reworks the encoder to use the VideoToolbox glue code that I published in CL 531863002. It also integrates the encoder with the sender implementation and contains build file changes. Please review this carefully. It should be the last big CL. Note that this depends on the CL for the media_cast component (see #581803003).
New patch set that rebases the build files on top of the latest version of 581803003, which defines the media_for_cast_ios component.
On 2014/09/19 18:08:38, jfroy wrote: > New patch set that rebases the build files on top of the latest version of > 581803003, which defines the media_for_cast_ios component. Starting...
Looks great. Could you write a simple unit test or two for this? Also, there's quite a bit of custom code you've written that generates the EncodedFrame::data result, so can there be a test that confirms the result can be decoded again? (You might consider mostly copying and modifying the test code from video_encoder_impl_unittest.cc.) Other comments, mostly small things: https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:180: } For safety: } else { NOTREACHED(); } Or, are there valid cases other than 1/2/4 bytes where they shouldn't be copied? https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:253: CompressionCallback, Style nit (function pointer): &H264VideoToolboxEncoder::CompressionCallback https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:286: static_cast<uint32_t>(video_config.start_bitrate)); Since bit rate change is not supported, should this be: (video_config.min_bitrate + video_config.max_bitrate) / 2 https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:472: format, Should this be pixel_format instead? https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:480: VideoFramePixelBufferReleaseCallback, nit: &VideoFramePixelBufferReleaseCallback ^^^ https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:498: uint32_t value) { This should probably be a signed int32_t instead, since this is how CFNumberCreate() will interpret it. Also, the callers of SetSessionProperty() wouldn't need the uint32 cast anymore. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:544: bool keyframe = CFDictionaryContainsKey( nit: bool keyframe = !CFDictionaryContainsKey(...); https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:564: // implementation compromises by setting the referenced frame ID to that of IMO, for clarity, the last sentence in this comment should be: "Cast doesn't support the concept of forward-referencing frame dependencies; so pretend that all frames are only decodable after their immediately preceding frame is decoded. This will ensure a Cast receiver only attempts to decode the frames sequentially and in order." https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:8: #include <string> This should be moved to the .cc file since std::string is only used there. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:35: CVPixelBufferPoolRef cv_pixel_buffer_pool() const; Style issue: This method is more complex than just reading a member variable, so it should use camel case: GetCvPixelBufferPool() https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... File media/cast/sender/video_sender.cc (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... media/cast/sender/video_sender.cc:74: if (!video_config.use_external_encoder && |use_external_encoder| is set to true when the hardware encoder should be used, so did you mean to omit the bang? Also, will video toolbox provide a software encoder on Mac OS, if the hardware support does not exist? Perhaps the value of |use_external_encoder| should be ignored here, and handled inside H264VideoToolboxEncoder::Initialize()? https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... media/cast/sender/video_sender.cc:100: video_encoder_.reset(new VideoEncoderImpl( On desktop Mac OS, |video_encoder_| will be recreated here (after being created on line 76 above). Perhaps lines 85-103 should be guarded with: if (!video_encoder_) { ... }
Thanks for the review. I'll submit a new patch set asap. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:180: } On 2014/09/24 00:10:13, miu wrote: > For safety: > > } else { > NOTREACHED(); > } > > Or, are there valid cases other than 1/2/4 bytes where they shouldn't be copied? No those are the only valid values per the H264 spec. I'll add the NOTREACHED branch. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:286: static_cast<uint32_t>(video_config.start_bitrate)); On 2014/09/24 00:10:13, miu wrote: > Since bit rate change is not supported, should this be: > > (video_config.min_bitrate + video_config.max_bitrate) / 2 Yeah probably. My producer code currently has min = max = start, so I hadn't noticed that. This implementation will eventually need better bitrate control (because the encoder needs to be torn down, I'm toying with the idea of bitrate bands, maybe 3-4, based on the video config values). https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:472: format, On 2014/09/24 00:10:13, miu wrote: > Should this be pixel_format instead? Wow yeah. Good catch. Thanks for nothing, compiler... https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:498: uint32_t value) { On 2014/09/24 00:10:12, miu wrote: > This should probably be a signed int32_t instead, since this is how > CFNumberCreate() will interpret it. Also, the callers of SetSessionProperty() > wouldn't need the uint32 cast anymore. Fair enough. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:564: // implementation compromises by setting the referenced frame ID to that of On 2014/09/24 00:10:13, miu wrote: > IMO, for clarity, the last sentence in this comment should be: > > "Cast doesn't support the concept of forward-referencing frame dependencies; so > pretend that all frames are only decodable after their immediately preceding > frame is decoded. This will ensure a Cast receiver only attempts to decode the > frames sequentially and in order." Alright. I'm also going to mention that forward references are explicitly disabled and that the encoder will emit frames essentially in presentation order (there can still be multiple backward references, but I think the profile and level I'm using restricts ref frames to 1). https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... File media/cast/sender/video_sender.cc (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... media/cast/sender/video_sender.cc:74: if (!video_config.use_external_encoder && On 2014/09/24 00:10:14, miu wrote: > |use_external_encoder| is set to true when the hardware encoder should be used, > so did you mean to omit the bang? > > Also, will video toolbox provide a software encoder on Mac OS, if the hardware > support does not exist? Perhaps the value of |use_external_encoder| should be > ignored here, and handled inside H264VideoToolboxEncoder::Initialize()? I understood |use_external_encoder| from the point of view of cast, which is to say that it should use the client-provided callbacks to allocate an external encoder managed by an instance of |ExternalVideoEncoder|. This is in fact what my first implementation used. However, |VideoEncodeAccelerator| (which is used by |ExternalVideoEncoder|) is a very poor interface for VideoToolbox (it forces unnecessary buffer allocations into unnecessarily large shared memory buffers). The basic |VideoEncoder| interface was much better, and this is what |H264VideoToolboxEncoder| implements. So from this point of view, this check is correct. VideoToolbox is opaque from the POV of the client. On OS X, it will indeed provide software encoding if there is no hardware encoder or if the hardware encoder is busy. On iOS, it only provides access to a hardware encoder and will fail with a "resource busy" error if someone else is already using it. This could be changed in the future by Apple on specific hardware. So in summary, |use_external_encoder| implies using the |ExternalVideoEncoder| class and using the client-provided vea and memory callbacks, which this implementation does not use. https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... media/cast/sender/video_sender.cc:100: video_encoder_.reset(new VideoEncoderImpl( On 2014/09/24 00:10:14, miu wrote: > On desktop Mac OS, |video_encoder_| will be recreated here (after being created > on line 76 above). Perhaps lines 85-103 should be guarded with: > > if (!video_encoder_) { > ... > } Oh man that is embarrassing. Thanks for catching that.
https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... File media/cast/sender/video_sender.cc (right): https://codereview.chromium.org/450693006/diff/280001/media/cast/sender/video... media/cast/sender/video_sender.cc:100: video_encoder_.reset(new VideoEncoderImpl( On 2014/09/24 00:10:14, miu wrote: > On desktop Mac OS, |video_encoder_| will be recreated here (after being created > on line 76 above). Perhaps lines 85-103 should be guarded with: > > if (!video_encoder_) { > ... > } I also wanted to mention why I did not integrate the VideoToolbox encoder in |VideoEncoderImpl|. The reason is that the |SoftwareVideoEncoder| interface used by |VideoEncoderImpl| imposes synchronous frame encoding and a dedicated video encoding thread. VideoToolbox already spawns its own threads and operates asynchronously. Furthermore, the encoder may not emit encoded frames until a few input frames have been provided, putting the encoder at risk of deadlocking: the implementation would have to block the video thread until the encoded frame corresponding to a submitted input frame is emitted, thus blocking submission of further input frames, thus potentially blocking encoded frames from being emitted. I also wanted to avoid the cost of an unnecessary thread and the latency of posting from the main thread to the video thread.
Patchset #16 (id:300001) has been deleted
All set with the impl, but still need unit tests. https://codereview.chromium.org/450693006/diff/360001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/360001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:294: // TODO(jfroy): implement better bitrate control Can you file a crbug for this, and put the link here?
Yep will do on the bug. Been working on tests for all the recent code (and on the acquisition part). On Tue, Sep 30, 2014 at 12:45 <miu@chromium.org> wrote: > All set with the impl, but still need unit tests. > > > https://codereview.chromium.org/450693006/diff/360001/ > media/cast/sender/h264_vt_encoder.cc > File media/cast/sender/h264_vt_encoder.cc (right): > > https://codereview.chromium.org/450693006/diff/360001/ > media/cast/sender/h264_vt_encoder.cc#newcode294 > media/cast/sender/h264_vt_encoder.cc:294 > <https://codereview.chromium.org/450693006/diff/360001/media/cast/sender/h264_...>: > // TODO(jfroy): implement > better bitrate control > Can you file a crbug for this, and put the link here? > > https://codereview.chromium.org/450693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patch set 23 adds 2 basic unit tests for the encoder. The first is similar to VideoEncoderImplTest/GeneratesKeyFrameThenOnlyDeltaFrames and checks the metadata of a sequence of encoded frames. The second uses FFmpegVideoDecoder to do an end-to-end encode-decode check of a sequence of test frames, using the PSNR function to validate. There are a few minor changes in the encoder itself as well, including new comments and adding color attachments required by OS X to buffers wrapping VideoFrames. Patch set 24 adds the unit tests file to BUILD.gn.
Well as I suspected the trybot Mac slaves don't support VideoToolbox, so the end to end test will always fail. I looked at the gtest documentation and there doesn't seem to be a way to disable a test at runtime (here by checking if VT is available). I don't know how I would do that check in gyp or gn to avoid compiling the test when it won't be able to run, or if that is even the right approach. I could just return from the test case if it can't run. There is no document on chromium.org detailing the policies of the project w/r to tests that need a specific environment.
Patchset #27 (id:570001) has been deleted
jfroy@chromium.org changed reviewers: + hclam@chromium.org
The latest patch set implements the video frame factory interface from https://codereview.chromium.org/688423003/ and reworks the unit tests to be an independent program / target (see https://crbug.com/431095).
miu, this is pretty much done. The unit tests are built from an independent target and I'll work with the infra people to get them running on Mac bots that support VideoToolbox.
lgtm % minor things: https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:7: #include <algorithm> Don't think you need this anymore. ...but you do need #include <queue> https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:189: base::ScopedCFTypeRef<CVPixelBufferPoolRef> pool) const base::ScopedCFTypeRef<CVPixelBufferPoolRef>& https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:20: typedef CoreMediaGlue::CMSampleBufferRef CMSampleBufferRef; nit (for simplicity): using CoreMediaGlue::CMSampleBufferRef; using VideoToolboxGlue::VTCompressionSessionRef; using VideoToolboxGlue::VTEncodeInfoFlags; https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.h:25: H264VideoToolboxEncoder(scoped_refptr<CastEnvironment> cast_environment, const scoped_refptr<CastEnvironment>& https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder_unittest.cc (right): https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:88: void Check(scoped_ptr<EncodedFrame> encoded_frame) { naming nit: How about CompareFrameWithExpected()? https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:89: EXPECT_LT(0u, expectations_.size()); This should be ASSERT_LT() to cause an early return before expectations_.front() is called. https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:150: void Check(const scoped_refptr<VideoFrame>& frame) { naming and use of ASSERT_LT() here too https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:168: scoped_ptr<FFmpegVideoDecoder> decoder_; nit: Don't need pointer-to-FFmpegVideoDecoder. Just: FFmpegVideoDecoder decoder_; ...and in the EndToEndFrameChecker ctor, add to the initializer list: decoder_(base::MessageLoop::current()->task_runner()) https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:241: scoped_refptr<CastEnvironment> cast_environment_; nit: It might not matter, but swap this line with the line above. When this class destructs, you want encoder_ destroyed before cast_environment_.
https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder.cc:7: #include <algorithm> On 2014/11/20 23:16:55, miu wrote: > Don't think you need this anymore. > > ...but you do need #include <queue> I don't need algo anymore indeed, but also don't think I need queue? git cl lint is not indicating so (but I don't know how reliable that is). https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... File media/cast/sender/h264_vt_encoder_unittest.cc (right): https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:88: void Check(scoped_ptr<EncodedFrame> encoded_frame) { On 2014/11/20 23:16:56, miu wrote: > naming nit: How about CompareFrameWithExpected()? Sure. https://codereview.chromium.org/450693006/diff/630001/media/cast/sender/h264_... media/cast/sender/h264_vt_encoder_unittest.cc:241: scoped_refptr<CastEnvironment> cast_environment_; On 2014/11/20 23:16:55, miu wrote: > nit: It might not matter, but swap this line with the line above. When this > class destructs, you want encoder_ destroyed before cast_environment_. It did not matter because of encoder_.reset(); in TearDown(), but I'll make the change anyways.
I think I've addressed your feedback in the last patch set, minus importing queue. I'll wait for your thumbs up before sending to the CQ.
The CQ bit was checked by miu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/450693006/650001
Message was sent while issue was closed.
Committed patchset #30 (id:650001)
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/3eb789bc38793698755e8e75d5850989696ff97d Cr-Commit-Position: refs/heads/master@{#305141} |
