Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847193003:
Convert Vp8 Rtp headers to frame references. (Closed)
|
Created:
4 years, 8 months ago by philipel Modified:
4 years, 8 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 Vp8 Rtp headers to frame references.
R=stefan@webrtc.org, pbos@webrtc.org
BUG=webrtc:5514
Committed: https://crrev.com/f41393376af08c838f51ab9ba1f6d100e4a2ef81
Cr-Commit-Position: refs/heads/master@{#12437}
Patch Set 1 #Patch Set 2 : #
Total comments: 53
Patch Set 3 : Feedback fixes. #Patch Set 4 : Continuously clean up map/vector/queues used to save recent state. #Patch Set 5 : Picture id start at the picture id of the first frame. #
Total comments: 47
Patch Set 6 : Feedback fixes/ #
Total comments: 5
Patch Set 7 : Feedback fixes/ #
Total comments: 8
Patch Set 8 : Feedback fixes. #Patch Set 9 : Rebase #
Messages
Total messages: 21 (6 generated)
some initial comments, please add someone who's more familiar with the code and packetization as well, I can't review the functionality very well https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:25: uint8_t num_references; size_t (unless this is bits put on the wire) https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:26: uint16_t references[5]; Comment what 5 comes from. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:135: // If all packets of the frame is contiuous, find the first packet of the continuous https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:138: int rindex = index; can you rename rindex to something I can understand? :) https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:148: std::unique_ptr<RtpFrameObject> frame( Follow-up maybe, but can these be reused so we don't have to heap allocate on every incoming frame? Maybe a TODO? https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:203: int start_index = frame->first_packet() % size_; size_t https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:216: int num_stashed_frames = stashed_frames_.size(); size_t for indexes and sizes https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:218: for (int f = 0; f < num_stashed_frames && !stashed_frames_.empty(); ++f) { size_t https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:218: for (int f = 0; f < num_stashed_frames && !stashed_frames_.empty(); ++f) { Comment on why this is not the same as while(!stashed_frames_.empty()) Also f as index? Pref i for generic indices, otherwise something meaningful as a name. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:229: int index = frame->first_packet() % size_; size_t https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:235: if (last_seq_num_for_kf_.size() > 5 || s/5/kSomeConstantName https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:274: int index = frame->first_packet() % size_; size_t https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:302: frame->num_references = 0; Shouldn't this line be true for every (generic) frame as well? Are there parts we can deduplicate? (Not obvious to me, just throwing it out there) https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:321: if (codec_header.temporalIdx == 0) { Should parts of this be split out to non-VP8 code that can be shared? This function is long and if handling temporal_idx will be the same for VP9 then it'd make sense to split this into generic functions https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:363: int index = frame->first_packet() % size_; size_t https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:52: static const uint8_t kMaxTemporalLayer = 5; I believe this can give linker errors since kMaxTemporalLayer won't be put into all objects. I think this triggers gtest macros, but it may not be a problem for you. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: // to determin whether a sequence of packets is continuous or not. determine https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:59: // The sequence number of the packet/ Replace / with . https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:62: // If this is the first packet of the frame. rename first_packet_of_frame maybe? https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:65: // If this is the last packet of the frame. last_packet_of_frame https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:75: bool frame_created = false; should this be in_use? https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:91: // Mark all slots used by |frame| as not used. s/used/previously used/ https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:98: // Retry to find the references for all frames that previously didn't have Retry finding references https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:103: void FindReferencesGeneric(std::unique_ptr<RtpFrameObject> frame) All of these FindReferences sound like they'd be const and return a list of references. Can you find a better name that indicates that it modifies state of frame/this object/both? ..HandleFrameReferences? Anything better? https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:110: // Updates all necessary state used to determin frame references determine https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:168: std::array<int16_t, kMaxTemporalLayer>, pref vector if this is not high-performance code (per https://chromium-cpp.appspot.com/), since vector is more widely in use And if it's high-performance, consider replacing std::map with a flat array that holds 0-255 same with not_yet_fully_received_ https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:171: bool initialized_ GUARDED_BY(crit_); Pref putting this closer to the sequence numbers which is what it's used with. Should we rename it has_received_first_packet, contains_first_packet or anything like that? https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:530: TEST_F(TestPacketBuffer, Vp8TemporalLayers_0212) { Can you put some comments on what these are doing specifically? I find _0212 hard to read since I'm less familiar with this part.
philipel@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:25: uint8_t num_references; On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t (unless this is bits put on the wire) Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:26: uint16_t references[5]; On 2016/04/01 14:24:44, pbos-webrtc wrote: > Comment what 5 comes from. Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:138: int rindex = index; On 2016/04/01 14:24:44, pbos-webrtc wrote: > can you rename rindex to something I can understand? :) Renamed it to |start_index|. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:148: std::unique_ptr<RtpFrameObject> frame( On 2016/04/01 14:24:44, pbos-webrtc wrote: > Follow-up maybe, but can these be reused so we don't have to heap allocate on > every incoming frame? Maybe a TODO? I think if we do that we add a lot of complexity for no measurable gain. A frame object is very small since it does not hold any packets. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:203: int start_index = frame->first_packet() % size_; On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:216: int num_stashed_frames = stashed_frames_.size(); On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t for indexes and sizes Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:218: for (int f = 0; f < num_stashed_frames && !stashed_frames_.empty(); ++f) { On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:218: for (int f = 0; f < num_stashed_frames && !stashed_frames_.empty(); ++f) { On 2016/04/01 14:24:44, pbos-webrtc wrote: > Comment on why this is not the same as > > while(!stashed_frames_.empty()) > > Also f as index? Pref i for generic indices, otherwise something meaningful as a > name. Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:229: int index = frame->first_packet() % size_; On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:235: if (last_seq_num_for_kf_.size() > 5 || On 2016/04/01 14:24:44, pbos-webrtc wrote: > s/5/kSomeConstantName Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:274: int index = frame->first_packet() % size_; On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:302: frame->num_references = 0; On 2016/04/01 14:24:44, pbos-webrtc wrote: > Shouldn't this line be true for every (generic) frame as well? > > Are there parts we can deduplicate? (Not obvious to me, just throwing it out > there) No, generic frames can have 0 or 1 reference. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:321: if (codec_header.temporalIdx == 0) { On 2016/04/01 14:24:44, pbos-webrtc wrote: > Should parts of this be split out to non-VP8 code that can be shared? This > function is long and if handling temporal_idx will be the same for VP9 then it'd > make sense to split this into generic functions VP9 wont need this functionality at all. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:363: int index = frame->first_packet() % size_; On 2016/04/01 14:24:44, pbos-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:52: static const uint8_t kMaxTemporalLayer = 5; On 2016/04/01 14:24:45, pbos-webrtc wrote: > I believe this can give linker errors since kMaxTemporalLayer won't be put into > all objects. I think this triggers gtest macros, but it may not be a problem for > you. Acknowledged. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:57: // to determin whether a sequence of packets is continuous or not. On 2016/04/01 14:24:44, pbos-webrtc wrote: > determine Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:59: // The sequence number of the packet/ On 2016/04/01 14:24:44, pbos-webrtc wrote: > Replace / with . Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:62: // If this is the first packet of the frame. On 2016/04/01 14:24:45, pbos-webrtc wrote: > rename first_packet_of_frame maybe? Seems a bit to long, and I think |frame_begin| accurately describes what the flag represents. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:65: // If this is the last packet of the frame. On 2016/04/01 14:24:45, pbos-webrtc wrote: > last_packet_of_frame Ditto https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:98: // Retry to find the references for all frames that previously didn't have On 2016/04/01 14:24:45, pbos-webrtc wrote: > Retry finding references Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:103: void FindReferencesGeneric(std::unique_ptr<RtpFrameObject> frame) On 2016/04/01 14:24:45, pbos-webrtc wrote: > All of these FindReferences sound like they'd be const and return a list of > references. Can you find a better name that indicates that it modifies state of > frame/this object/both? > > > ..HandleFrameReferences? Anything better? Renamed them to ManageFrameXXX. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:110: // Updates all necessary state used to determin frame references On 2016/04/01 14:24:45, pbos-webrtc wrote: > determine Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:168: std::array<int16_t, kMaxTemporalLayer>, On 2016/04/01 14:24:45, pbos-webrtc wrote: > pref vector if this is not high-performance code (per > https://chromium-cpp.appspot.com/), since vector is more widely in use > > And if it's high-performance, consider replacing std::map with a flat array that > holds 0-255 > > same with not_yet_fully_received_ I think map is the correct datastructure to use here since I want to use a custom comparator to find elements. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:171: bool initialized_ GUARDED_BY(crit_); On 2016/04/01 14:24:45, pbos-webrtc wrote: > Pref putting this closer to the sequence numbers which is what it's used with. > Should we rename it has_received_first_packet, contains_first_packet or anything > like that? Done. https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1847193003/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:530: TEST_F(TestPacketBuffer, Vp8TemporalLayers_0212) { On 2016/04/01 14:24:45, pbos-webrtc wrote: > Can you put some comments on what these are doing specifically? I find _0212 > hard to read since I'm less familiar with this part. Done.
First set of comments https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:209: if (codec_type == kVideoCodecVP8) { switch() https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:260: static_cast<uint16_t>(frame->first_packet() - 1)) { I'm reading this as if frame must be next frame after the key frame, otherwise we stash it. Can't there be delta frames in between? https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); I find this a bit strange. Why do we assign a packet to a picture_id? We should clarify the code here in some way. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:270: frame->references[0] = seq_num_it->second; Always referring to the key frame? I guess this is related to my question on line 260. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:309: // Clean up info for base layers that are to old. too old https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:310: uint8_t old_tl0_pic_idx = codec_header.tl0PicIdx - 10; Why is 10 frames back too old? Wouldn't it be better to define too old as being X frames older than the current _if_ a key frame exists which is newer? Otherwise we might as well wait. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:318: clean_frames_to); I think we should break out the clean up functionality to a separeate function https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:328: ? codec_header.tl0PicIdx - 1 Should this do Add<...>(codec_header.tl0PicIdx, -1)? https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:346: frame->references[0] = layer_info_it->second[0]; Is it guaranteed that there isn't a gap between "frame" and layer_info_it->second[0]? I.e., do we know at this point that we have received all tl0 frames prior to "frame"? https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:365: frame->references[layer] = layer_info_it->second[layer]; Same question here as on line 346, but I see below that you ensure we wait for any frames in between. Should that be done at line 347 too? And should we really update frame->references[layer] here if we don't know if it's the right reference we're setting it to? (I assume we're waiting below because it might not be the correct reference) https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:148: last_seq_num_for_kf_ GUARDED_BY(crit_); I think this is a bit confusing. If I have received a key frame with last seq num 10, and then I receive a frame with last seq num 11, will last_seq_num_for_kf_[10] == 11 then? And if I receive another frame with last seq num 12 it will be updated to 12? https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:151: // that has not yet been fully received. that have not yet https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:152: int last_picture_id_ GUARDED_BY(crit_); Is this the last received picture id or the last completed picture id? https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:158: // Frames earlier than the last received frame that has not yet been ...that have not yet... https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:163: // Frames that has been fully received but didn't have all the information Frames that have been https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:164: // needed to determine its references. their references.
https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:209: if (codec_type == kVideoCodecVP8) { On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > switch() If I implement a switch case I am forced by the compiler to implement a case for all possible values, which are: kRtpVideoNone, kRtpVideoGeneric, kRtpVideoVp8, kRtpVideoVp9, kRtpVideoH264, default https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:260: static_cast<uint16_t>(frame->first_packet() - 1)) { On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > I'm reading this as if frame must be next frame after the key frame, otherwise > we stash it. Can't there be delta frames in between? There can be delta frames in between. On line 271 the sequence number (picture id) in the map is updated. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > I find this a bit strange. Why do we assign a packet to a picture_id? We should > clarify the code here in some way. Added comment which explains why. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:270: frame->references[0] = seq_num_it->second; On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Always referring to the key frame? I guess this is related to my question on > line 260. Since we update the picture id stored in the map this will always refer back to the previous frame for some keyframe. I updated line 271 to: seq_num_it->second = frame->picture_id; which hopefully makes it clearer. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:309: // Clean up info for base layers that are to old. On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > too old Done. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:310: uint8_t old_tl0_pic_idx = codec_header.tl0PicIdx - 10; On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Why is 10 frames back too old? Wouldn't it be better to define too old as being > X frames older than the current _if_ a key frame exists which is newer? > Otherwise we might as well wait. The value 10 is just an arbitrary number that is "big enough". Since a new entry is added to |layer_info_| for every frame on tl0 I can't see that making the definition of "too old" depend on whether we have newer keyframes or not to make any difference. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:318: clean_frames_to); On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > I think we should break out the clean up functionality to a separeate function Hmm... I think the interface of such a function would become kind of messy... https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:328: ? codec_header.tl0PicIdx - 1 On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Should this do Add<...>(codec_header.tl0PicIdx, -1)? Not needed in this case since tl0PicIdx wraps at 255. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:346: frame->references[0] = layer_info_it->second[0]; On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Is it guaranteed that there isn't a gap between "frame" and > layer_info_it->second[0]? I.e., do we know at this point that we have received > all tl0 frames prior to "frame"? On line 327 we check if this is a frame on temporal layer 0, and if so look for the layer info for tl0PicIdx - 1. If we can't find it then we stash this frame (line 332). So at this point we are sure to have the base layer frame needed for this frame. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:365: frame->references[layer] = layer_info_it->second[layer]; On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Same question here as on line 346, but I see below that you ensure we wait for > any frames in between. Should that be done at line 347 too? And should we really > update frame->references[layer] here if we don't know if it's the right > reference we're setting it to? (I assume we're waiting below because it might > not be the correct reference) Updating the references here doesn't really matter since they will all be correctly assigned when we have all information needed and this frame is retried. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:148: last_seq_num_for_kf_ GUARDED_BY(crit_); On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > I think this is a bit confusing. If I have received a key frame with last seq > num 10, and then I receive a frame with last seq num 11, will > last_seq_num_for_kf_[10] == 11 then? And if I receive another frame with last > seq num 12 it will be updated to 12? That is exactly how it works. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:151: // that has not yet been fully received. On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > that have not yet Done. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:152: int last_picture_id_ GUARDED_BY(crit_); On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Is this the last received picture id or the last completed picture id? The |last_picture_id_| is the id of the last frame that has received all of its packets. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:158: // Frames earlier than the last received frame that has not yet been On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > ...that have not yet... Done. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:163: // Frames that has been fully received but didn't have all the information On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > Frames that have been Done. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:164: // needed to determine its references. On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > their references. Done.
ping
https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:209: if (codec_type == kVideoCodecVP8) { On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > switch() > > If I implement a switch case I am forced by the compiler to implement a case for > all possible values, which are: > > kRtpVideoNone, > kRtpVideoGeneric, > kRtpVideoVp8, > kRtpVideoVp9, > kRtpVideoH264, > default I think that could be a good thing. If a new type is added we won't forget about this. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > I find this a bit strange. Why do we assign a packet to a picture_id? We > should > > clarify the code here in some way. > > Added comment which explains why. But maybe last_packet() should return last_picture_id() or something? To make it clear it's actually a picture_id https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:315: uint16_t old_picture_id = Subtract<kPicIdLength>(frame->picture_id, 20); Name this constant, and the same with 10 above. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:318: clean_frames_to); On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > I think we should break out the clean up functionality to a separeate function > > Hmm... I think the interface of such a function would become kind of messy... Maybe. I was thinking something simple, like CleanupOld(picture_id, tl0_pic_idx); not super important https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:328: ? codec_header.tl0PicIdx - 1 On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > Should this do Add<...>(codec_header.tl0PicIdx, -1)? > > Not needed in this case since tl0PicIdx wraps at 255. Acknowledged. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:346: frame->references[0] = layer_info_it->second[0]; On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > Is it guaranteed that there isn't a gap between "frame" and > > layer_info_it->second[0]? I.e., do we know at this point that we have received > > all tl0 frames prior to "frame"? > > On line 327 we check if this is a frame on temporal layer 0, and if so look for > the layer info for tl0PicIdx - 1. If we can't find it then we stash this frame > (line 332). > > So at this point we are sure to have the base layer frame needed for this frame. Acknowledged. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:365: frame->references[layer] = layer_info_it->second[layer]; On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > Same question here as on line 346, but I see below that you ensure we wait for > > any frames in between. Should that be done at line 347 too? And should we > really > > update frame->references[layer] here if we don't know if it's the right > > reference we're setting it to? (I assume we're waiting below because it might > > not be the correct reference) > > Updating the references here doesn't really matter since they will all be > correctly assigned when we have all information needed and this frame is > retried. So should we only update the references when we actually know that they are correct, e.g., on line 377? https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_object.h:14: #include <cstddef> stddef.h is used 65 times in webrtc, cstddef is used 11 times. Should we use stddef.h for consistency? Same goes for stdint.h. Typically I think typedefs.h is used instead of stdint.h, but I don't think there's a reason for that any longer. https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_object.h:29: uint16_t references[kMaxFrameReferences]; Maybe use a vector instead of a size_t and an array? https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:235: void PacketBuffer::ManageFrameGeneric( Did we decide on whether to break out the ManageFrameGeneric/Vp8/Vp9/H264 functionality to its own classes or something like that? Maybe something like: class PacketBuffer { ... private: std::unique_ptr<FrameManager> frame_manager_; }; class GenericFrameManager : public FrameManager { ... } class Vp8FrameManager : public GenericFrameManager { ... } etc. Might require some refactoring of the ManageFrame*() methods, but it would make the code cleaner I think. It may also allow us to move Vp8 only members into the Vp8FrameManager, etc.
https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:209: if (codec_type == kVideoCodecVP8) { On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > On 2016/04/07 13:38:42, philipel wrote: > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > switch() > > > > If I implement a switch case I am forced by the compiler to implement a case > for > > all possible values, which are: > > > > kRtpVideoNone, > > kRtpVideoGeneric, > > kRtpVideoVp8, > > kRtpVideoVp9, > > kRtpVideoH264, > > default > > I think that could be a good thing. If a new type is added we won't forget about > this. Switched to switch case. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > On 2016/04/07 13:38:42, philipel wrote: > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > I find this a bit strange. Why do we assign a packet to a picture_id? We > > should > > > clarify the code here in some way. > > > > Added comment which explains why. > > But maybe last_packet() should return last_picture_id() or something? To make it > clear it's actually a picture_id The function last_packet simply returns the sequence number of the last packet of this frame. In the generic case we use the sequence number of the last packet as the picture_id. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:315: uint16_t old_picture_id = Subtract<kPicIdLength>(frame->picture_id, 20); On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > Name this constant, and the same with 10 above. Done. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:365: frame->references[layer] = layer_info_it->second[layer]; On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > On 2016/04/07 13:38:42, philipel wrote: > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > Same question here as on line 346, but I see below that you ensure we wait > for > > > any frames in between. Should that be done at line 347 too? And should we > > really > > > update frame->references[layer] here if we don't know if it's the right > > > reference we're setting it to? (I assume we're waiting below because it > might > > > not be the correct reference) > > > > Updating the references here doesn't really matter since they will all be > > correctly assigned when we have all information needed and this frame is > > retried. > > So should we only update the references when we actually know that they are > correct, e.g., on line 377? Changed so that the check occurs before the assignment. https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_object.h:14: #include <cstddef> On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > stddef.h is used 65 times in webrtc, cstddef is used 11 times. Should we use > stddef.h for consistency? > > Same goes for stdint.h. Typically I think typedefs.h is used instead of > stdint.h, but I don't think there's a reason for that any longer. Done. https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:235: void PacketBuffer::ManageFrameGeneric( On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > Did we decide on whether to break out the ManageFrameGeneric/Vp8/Vp9/H264 > functionality to its own classes or something like that? Maybe something like: > > class PacketBuffer { > ... > > private: > std::unique_ptr<FrameManager> frame_manager_; > }; > > class GenericFrameManager : public FrameManager { ... } > class Vp8FrameManager : public GenericFrameManager { ... } > etc. > > Might require some refactoring of the ManageFrame*() methods, but it would make > the code cleaner I think. It may also allow us to move Vp8 only members into the > Vp8FrameManager, etc. Yes, I will break out this functionality into separate classes after this CL and the Cl for "Vp9 to references" as well.
https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:260: static_cast<uint16_t>(frame->first_packet() - 1)) { On 2016/04/07 13:38:42, philipel wrote: > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > I'm reading this as if frame must be next frame after the key frame, otherwise > > we stash it. Can't there be delta frames in between? > > There can be delta frames in between. On line 271 the sequence number (picture > id) in the map is updated. I was confused about the last_seq_num_for_kf_. I interpreted it as if it contained the last sequence numbers for the packets belonging to a key frame, but it contains the last sequence numbers of each group of independently decodable frames. Maybe we can rename it as such? GOP (group of pictures) is often used in video coding when talking about those things. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); On 2016/04/19 11:52:07, philipel wrote: > On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > > On 2016/04/07 13:38:42, philipel wrote: > > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > > I find this a bit strange. Why do we assign a packet to a picture_id? We > > > should > > > > clarify the code here in some way. > > > > > > Added comment which explains why. > > > > But maybe last_packet() should return last_picture_id() or something? To make > it > > clear it's actually a picture_id > > The function last_packet simply returns the sequence number of the last packet > of this frame. In the generic case we use the sequence number of the last packet > as the picture_id. Ah, call it last_packet_sequence_number() or last_sequence_number()? My point is that it doesn't return a Packet. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:221: case kVideoCodecULPFEC : RED and ULPFEC shouldn't end up here at all, so let's DCHECK on that not happening. Same for Unknown I tihnk? https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:233: // Clean up stashed frames if there are to many. too many https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:262: TEST_F(TestPacketBuffer, GetBitstreamFromFrame) { Can we have a test with at least 3 generic frames where they come in reordered: frame1: key frame2: delta frame4: delta frame3: delta Or something like that? Would be good for VP8 too. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:687: InsertVp8(current++, false, false, true , false, pid + f, 0 , f); Remove some misplaced whitespaces here and above.
https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:260: static_cast<uint16_t>(frame->first_packet() - 1)) { On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > On 2016/04/07 13:38:42, philipel wrote: > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > I'm reading this as if frame must be next frame after the key frame, > otherwise > > > we stash it. Can't there be delta frames in between? > > > > There can be delta frames in between. On line 271 the sequence number (picture > > id) in the map is updated. > > I was confused about the last_seq_num_for_kf_. I interpreted it as if it > contained the last sequence numbers for the packets belonging to a key frame, > but it contains the last sequence numbers of each group of independently > decodable frames. Maybe we can rename it as such? GOP (group of pictures) is > often used in video coding when talking about those things. Renamed it to |last_seq_num_gop_|. https://codereview.webrtc.org/1847193003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:268: frame->picture_id = frame->last_packet(); On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > On 2016/04/19 11:52:07, philipel wrote: > > On 2016/04/19 10:38:19, stefan-webrtc (holmer) wrote: > > > On 2016/04/07 13:38:42, philipel wrote: > > > > On 2016/04/07 12:37:04, stefan-webrtc (holmer) wrote: > > > > > I find this a bit strange. Why do we assign a packet to a picture_id? We > > > > should > > > > > clarify the code here in some way. > > > > > > > > Added comment which explains why. > > > > > > But maybe last_packet() should return last_picture_id() or something? To > make > > it > > > clear it's actually a picture_id > > > > The function last_packet simply returns the sequence number of the last packet > > of this frame. In the generic case we use the sequence number of the last > packet > > as the picture_id. > > Ah, call it last_packet_sequence_number() or last_sequence_number()? My point is > that it doesn't return a Packet. True, renamed |last_packet()| to |last_seq_num()| and |first_packet| to |first_seq_num()|. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:221: case kVideoCodecULPFEC : On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > RED and ULPFEC shouldn't end up here at all, so let's DCHECK on that not > happening. Same for Unknown I tihnk? Done. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:233: // Clean up stashed frames if there are to many. On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > too many Done. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:262: TEST_F(TestPacketBuffer, GetBitstreamFromFrame) { On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > Can we have a test with at least 3 generic frames where they come in reordered: > > frame1: key > frame2: delta > frame4: delta > frame3: delta > > Or something like that? Would be good for VP8 too. Done. https://codereview.webrtc.org/1847193003/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:687: InsertVp8(current++, false, false, true , false, pid + f, 0 , f); On 2016/04/19 12:24:29, stefan-webrtc (holmer) wrote: > Remove some misplaced whitespaces here and above. Done.
lgtm
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/1847193003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847193003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847193003/160001
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)
Description was changed from ========== Convert Vp8 Rtp headers to frame references. R=pbos@webrtc.org BUG=webrtc:5514 ========== to ========== Convert Vp8 Rtp headers to frame references. R=stefan@webrtc.org, pbos@webrtc.org BUG=webrtc:5514 Committed: https://chromium.googlesource.com/external/webrtc/+/f41393376af08c838f51ab9ba... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as f41393376af08c838f51ab9ba1f6d100e4a2ef81 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Convert Vp8 Rtp headers to frame references. R=stefan@webrtc.org, pbos@webrtc.org BUG=webrtc:5514 Committed: https://chromium.googlesource.com/external/webrtc/+/f41393376af08c838f51ab9ba... ========== to ========== Convert Vp8 Rtp headers to frame references. R=stefan@webrtc.org, pbos@webrtc.org BUG=webrtc:5514 Committed: https://crrev.com/f41393376af08c838f51ab9ba1f6d100e4a2ef81 Cr-Commit-Position: refs/heads/master@{#12437} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f41393376af08c838f51ab9ba1f6d100e4a2ef81 Cr-Commit-Position: refs/heads/master@{#12437} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
