Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903523003:
Convert Vp9 Rtp headers to frame references. (Closed)
|
Created:
4 years, 8 months ago by philipel Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionConvert Vp9 Rtp headers to frame references.
R=mflodman@webrtc.org, stefan@webrtc.org
BUG=webrtc:5514
Committed: https://crrev.com/a1059874a6d6c5e4b963ac8cf74bf20ff0ecc8f7
Cr-Commit-Position: refs/heads/master@{#12660}
Patch Set 1 #Patch Set 2 : Added comments. #
Total comments: 37
Patch Set 3 : Feedback fixes. #
Total comments: 28
Patch Set 4 : Feedback fixes and format. #Patch Set 5 : Rebase #Patch Set 6 : Initialize scalability structure in Vp9GofInserOneFrame. #
Total comments: 1
Patch Set 7 : Includes in alphabetic order. #
Messages
Total messages: 32 (14 generated)
Sorry for the delay https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:125: case kTemporalStructureMode4: I think you should make send-side changes in a separate CL. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:33: uint16_t picture_id; I think it would be good to add a comment somewhere here that explains that picture_id will be inferred (and short description of how?) for codecs which don't have it. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:323: if (AheadOf<uint16_t, kPicIdLength>(frame->picture_id, last_picture_id_)) { Do we have a unittest which would have caught this? Otherwise please add one. Or maybe this wasn't a bug, but a change of internal representation? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:453: frame->picture_id = codec_header.picture_id % kPicIdLength; kPictureIdLength https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:461: // GLORIUS flexible mode! I don't think we need this comment... :) https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:464: for (size_t r = 0; r < frame->num_references; ++r) { use index i rather than r. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:475: RTC_DCHECK_EQ(0, codec_header.spatial_idx); I don't think we'd want to crash if someone gives us an incorrect bitstream with an ss in a non tl0 frame, so please just log a warning. Also, you're actually checking spatial_idx and not temporal_idx. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:486: // Clean up info for base layers that are to old. too old https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:507: stashed_frames_.emplace(std::move(frame)); What happens if we have already deleted the ss that this frame depends on? Will stashed_frames_ ever be emptied and a key frame requested? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:523: // Sync frames only depend on their tl0_pic_idx.W Erase W. And is this comment true? I think a frame with the U flag set and temporal idx T means that it's possible to switch up T+N, but it doesn't mean that following frames only will depend on tl0, only that they will be limited to depending on frames with temporal idx <= T. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:531: RTC_DCHECK((AheadOrAt<uint16_t, kPicIdLength>(frame->picture_id, Is this a safe DCHECK if someone produces a bit stream just to make us crash? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:535: frame->picture_id); May not be correctly indented https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:540: for (size_t r = 0; r < frame->num_references; ++r) { index i https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:542: frame->picture_id); Indentation. And actually, you have already computed this diff, right? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:543: gof_idx = diff % gof->num_frames_in_gof; This too, if I'm not mistaken https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: static const int kMaxGofSaved = 5; This constant seems to only be used in the .cc file. Move it there in that case. Maybe other constants can be moved too? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: static const int kMaxGofSaved = 5; I wonder if 5 GOFs will actually be enough... It can happen that we get several key frames in a short period of time if we're unlucky. 5 within 1 second could happen. I'd be more comfortable with maybe 15? https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:59: }; Not a big fan of this, I don't think it makes the code much easier to read. Rather it confuses the reader.
https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:125: case kTemporalStructureMode4: On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > I think you should make send-side changes in a separate CL. I added this SS in order to test the sync bit and I don't think I can use any other SS for that. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:33: uint16_t picture_id; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > I think it would be good to add a comment somewhere here that explains that > picture_id will be inferred (and short description of how?) for codecs which > don't have it. Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:323: if (AheadOf<uint16_t, kPicIdLength>(frame->picture_id, last_picture_id_)) { On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > Do we have a unittest which would have caught this? Otherwise please add one. > > Or maybe this wasn't a bug, but a change of internal representation? This was not a bug, I simply wanted to test what would happen if we used 9 or 13 bits instead of 7 bits for picture ids. If we ever want to use only 15 bit picture id we can simply change kPicIdLength to 1 << 15. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:453: frame->picture_id = codec_header.picture_id % kPicIdLength; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > kPictureIdLength I prefer not, there will be many lines that are already hard to format that will become longer that 80 chars. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:461: // GLORIUS flexible mode! On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > I don't think we need this comment... :) Removed :P https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:464: for (size_t r = 0; r < frame->num_references; ++r) { On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > use index i rather than r. Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:475: RTC_DCHECK_EQ(0, codec_header.spatial_idx); On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > I don't think we'd want to crash if someone gives us an incorrect bitstream with > an ss in a non tl0 frame, so please just log a warning. Also, you're actually > checking spatial_idx and not temporal_idx. SS are now ignored if they are not sent on a base layer frame. Also log a warning if that happens. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:486: // Clean up info for base layers that are to old. On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > too old Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:507: stashed_frames_.emplace(std::move(frame)); On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > What happens if we have already deleted the ss that this frame depends on? Will > stashed_frames_ ever be emptied and a key frame requested? RetryStashedFrames have built in clean up logic which prevents frames from building up. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:523: // Sync frames only depend on their tl0_pic_idx.W On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > Erase W. > > And is this comment true? I think a frame with the U flag set and temporal idx T > means that it's possible to switch up T+N, but it doesn't mean that following > frames only will depend on tl0, only that they will be limited to depending on > frames with temporal idx <= T. I was thinking of sync frames for Vp8 for some reason. Fixed now. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:531: RTC_DCHECK((AheadOrAt<uint16_t, kPicIdLength>(frame->picture_id, On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > Is this a safe DCHECK if someone produces a bit stream just to make us crash? What this check do is to make sure we don't select a SS that is newer than the SS that this frame belongs to, so it's safe (programming error). https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:535: frame->picture_id); On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > May not be correctly indented Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:540: for (size_t r = 0; r < frame->num_references; ++r) { On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > index i Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:542: frame->picture_id); On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > Indentation. > > And actually, you have already computed this diff, right? Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:543: gof_idx = diff % gof->num_frames_in_gof; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > This too, if I'm not mistaken Done. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: static const int kMaxGofSaved = 5; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > This constant seems to only be used in the .cc file. Move it there in that case. > Maybe other constants can be moved too? This is used on line 193 in the .h file. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: static const int kMaxGofSaved = 5; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > I wonder if 5 GOFs will actually be enough... It can happen that we get several > key frames in a short period of time if we're unlucky. 5 within 1 second could > happen. I'd be more comfortable with maybe 15? Increased to 15. https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:59: }; On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > Not a big fan of this, I don't think it makes the code much easier to read. > Rather it confuses the reader. I agree it's somewhat hacky, but the problem is if I use "true" and "false" many lines becomes longer than 80 chars and I can't insert packets in a nice "matrix" format.
https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1903523003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:59: }; On 2016/04/28 09:40:42, philipel wrote: > On 2016/04/26 08:15:12, stefan-webrtc (holmer) wrote: > > Not a big fan of this, I don't think it makes the code much easier to read. > > Rather it confuses the reader. > > I agree it's somewhat hacky, but the problem is if I use "true" and "false" many > lines becomes longer than 80 chars and I can't insert packets in a nice "matrix" > format. Not sure it's worth to keep that then, sooner or later nice formatting breaks anyway. Also, you can't run git cl format now, right? https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:33: // have to be constructed from the header data relevant to that coded. to that codec https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:480: scalability_structures_[current_ss_idx_] = codec_header.gof; If you do auto inserted = scalability_structures_.insert(...); you can directly refer to the inserted element below using inserted.first. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:484: &scalability_structures_[current_ss_idx_]); git cl format https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:500: GofInfoVP9* gof = gof_info_.find(codec_header.tl0_pic_idx)->second.second; It would be nice to clean this up and rename it GofInfoVp9. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:544: Remove empty line https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:553: uint8_t diff = ForwardDiff<uint16_t, kPicIdLength>(gof->pid_start, This is a bit confusing to me. Why is the diff stored as uint8_t if the diff is computed modulo kPicIdLength? Can we store it as an int instead? https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:555: uint8_t gof_idx = diff % gof->num_frames_in_gof; Can we make gof_idx a size_t? There's no point in making it an uint8_t, right? https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:566: codec_header.temporal_idx, This doesn't look correctly formatted. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:579: uint8_t temporal_idx = gof.temporal_idx[gof_idx]; Should we use int for all of these instead? Just to indicate that they won't be used for any modulo operations. 8 bits isn't the right size for them anyway, right? https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:582: // the intervall (|ref_pid|, |picture_id|) in any of the lower temporal interval https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:603: // If there is a gap, find which temporal layers the missing framess frames https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:604: // belongs to and add the frame as missing for that temporal layer. belong to https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:632: while (up_switch_it != up_switch_.end() && I'd use a for loop here instead
https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:33: // have to be constructed from the header data relevant to that coded. On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > to that codec Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:480: scalability_structures_[current_ss_idx_] = codec_header.gof; On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > If you do > auto inserted = scalability_structures_.insert(...); > you can directly refer to the inserted element below using inserted.first. The member |scalability_structures_| is an std::array. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:484: &scalability_structures_[current_ss_idx_]); On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > git cl format Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:500: GofInfoVP9* gof = gof_info_.find(codec_header.tl0_pic_idx)->second.second; On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > It would be nice to clean this up and rename it GofInfoVp9. Not sure what you mean. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:544: On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > Remove empty line Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:553: uint8_t diff = ForwardDiff<uint16_t, kPicIdLength>(gof->pid_start, On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > This is a bit confusing to me. Why is the diff stored as uint8_t if the diff is > computed modulo kPicIdLength? Can we store it as an int instead? You are right that an uint8_t is to small, but the diff is always positive so i changed it to a size_t. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:555: uint8_t gof_idx = diff % gof->num_frames_in_gof; On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > Can we make gof_idx a size_t? There's no point in making it an uint8_t, right? Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:566: codec_header.temporal_idx, On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > This doesn't look correctly formatted. Formated. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:579: uint8_t temporal_idx = gof.temporal_idx[gof_idx]; On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > Should we use int for all of these instead? Just to indicate that they won't be > used for any modulo operations. 8 bits isn't the right size for them anyway, > right? Changed to size_t. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:582: // the intervall (|ref_pid|, |picture_id|) in any of the lower temporal On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > interval Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:603: // If there is a gap, find which temporal layers the missing framess On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > frames Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:604: // belongs to and add the frame as missing for that temporal layer. On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > belong to Done. https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:632: while (up_switch_it != up_switch_.end() && On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > I'd use a for loop here instead Done.
lgtm https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:480: scalability_structures_[current_ss_idx_] = codec_header.gof; On 2016/05/04 09:52:26, philipel wrote: > On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > > If you do > > auto inserted = scalability_structures_.insert(...); > > you can directly refer to the inserted element below using inserted.first. > > The member |scalability_structures_| is an std::array. Ah, thanks https://codereview.webrtc.org/1903523003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:500: GofInfoVP9* gof = gof_info_.find(codec_header.tl0_pic_idx)->second.second; On 2016/05/04 09:52:26, philipel wrote: > On 2016/05/04 09:09:50, stefan-webrtc (holmer) wrote: > > It would be nice to clean this up and rename it GofInfoVp9. > > Not sure what you mean. I was refering to GofInfoVP9 -> GofInfoVp9. Feel free to ignore.
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903523003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903523003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7258) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7244) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9582) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9506) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14787) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13440) linux_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14409) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11137) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2931) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9152) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14890)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1903523003/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903523003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903523003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1830) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
philipel@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, PTAL at common_types.h, need owners approval.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1903523003/#ps100001 (title: "Initialize scalability structure in Vp9GofInserOneFrame.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903523003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903523003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5336)
One nit, then lgtm. https://codereview.webrtc.org/1903523003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1903523003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.h:15: #include <utility> Alphabetic order.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1903523003/#ps120001 (title: "Includes in alphabetic order.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903523003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903523003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Convert Vp9 Rtp headers to frame references. R=stefan@webrtc.org BUG=webrtc:5514 ========== to ========== Convert Vp9 Rtp headers to frame references. R=mflodman@webrtc.org, stefan@webrtc.org BUG=webrtc:5514 Committed: https://crrev.com/a1059874a6d6c5e4b963ac8cf74bf20ff0ecc8f7 Cr-Commit-Position: refs/heads/master@{#12660} ==========
Message was sent while issue was closed.
Description was changed from ========== Convert Vp9 Rtp headers to frame references. R=mflodman@webrtc.org, stefan@webrtc.org BUG=webrtc:5514 Committed: https://crrev.com/a1059874a6d6c5e4b963ac8cf74bf20ff0ecc8f7 Cr-Commit-Position: refs/heads/master@{#12660} ========== to ========== Convert Vp9 Rtp headers to frame references. R=mflodman@webrtc.org, stefan@webrtc.org BUG=webrtc:5514 Committed: https://chromium.googlesource.com/external/webrtc/+/a1059874a6d6c5e4b963ac8cf... ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a1059874a6d6c5e4b963ac8cf74bf20ff0ecc8f7 Cr-Commit-Position: refs/heads/master@{#12660}
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as a1059874a6d6c5e4b963ac8cf74bf20ff0ecc8f7 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
