Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322263002:
Frame continuity is now tested as soon as a frame is inserted into the FrameBuffer. (Closed)
DescriptionFrame continuity is now tested as soon as a frame is inserted into the FrameBuffer.
Since we want to stop sending NACKS for frames not needed to keep the stream
decodable we must know which frames that are continuous or not.
BUG=webrtc:5514
R=danilchap@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/e0b2f154170ae2bfc11ddfede3241101748dfec3
Patch Set 1 #Patch Set 2 : Added logging, comments and unittest. #Patch Set 3 : More comments. #
Total comments: 21
Patch Set 4 : Feedback fixes #
Total comments: 42
Patch Set 5 : Feedback Fixes. #
Total comments: 20
Patch Set 6 : Feeback fixes. #Patch Set 7 : inter_layer_predicted continuity fix + unittest #
Total comments: 2
Patch Set 8 : Avoid adding unnecessary backwards references for inter layer predicted frames. #
Total comments: 48
Patch Set 9 : Feedback #
Total comments: 1
Messages
Total messages: 30 (10 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:55: decltype(frames_.end()) next_frame_it; may be add using Frames = std::map<FrameKey, FrameObject>; into .h file right after all struct definitions, then you can write here more explicit: Frames::iterator next_frame_it; (I think decltype more when you are not sure what type is, i.e. template metaprogramming. Here you know exact type, it is just too long. Creating alias with using feels more appropriate in this case) https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:69: const FrameInfo& frame_info = frame_history_.find(frame_key)->second; frame_history_ can through out frames when there are too many of them. Can that lead to situation where you have key in frames_ but not in frame_history_? i.e. may be add a DCHECK or check frame_key is in the history. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:166: while (frame_history_.size() > kMaxNumHistoryFrames || probably cleaner to have two loops like before. The erase code is small enough not to worry about code duplication. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:174: auto info = frame_history_.insert(std::make_pair(key, FrameInfo())).first; since your ignore .second anyway, may be cleaner FrameInfo* info = &frame_history_[key]; https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:185: if (ref_info->second.decoded) { do you distinguish decoded (returned by NextFrame) and decodable (has enough packet to decode a frame, enough packet for NextFrame to return it)? what does continuous mean? decodable? https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:250: auto ref_info = frame_history_.find(info->second.dependent_frames[d]); You sure info->second.dependent_frames[d] is in the history? https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:104: bool operator()(const FrameKey& f1, const FrameKey& f2) const { If you add this operator as a bool operator<(const FrameKey& other) const into FrameKey you will have one helper structure less. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:105: if (f1.picture_id == f2.picture_id) what is the reason for moving implementation into .h from .cc? https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:368: TEST_F(TestFrameBuffer2, LastContinuousFrameSingleStream) { It helps when test name describes both scenario and expected result. If you have several scenarios (look like you do) - make several smaller tests. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:378: CheckLastContinuousFrame(0, pid); May be make InsertFrame return the result of buffer_.InsertFrame and merge InsertFrame & CheckLastContinuousFrame calls. Less fixture state should make more clear what this 5 calls refer to: EXPECT_EQ(static_cast<int>(pid), InsertFrame(pid, 0, ts, false)); EXPECT_EQ(static_cast<int>(pid), InsertFrame(pid + 2, 0, ts, false, pid + 1); ... https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:383: } May be add a test case where InsertFrame return -1
https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:55: decltype(frames_.end()) next_frame_it; On 2016/09/12 12:20:14, danilchap wrote: > may be add > using Frames = std::map<FrameKey, FrameObject>; > into .h file right after all struct definitions, > then you can write here more explicit: > Frames::iterator next_frame_it; > > (I think decltype more when you are not sure what type is, i.e. template > metaprogramming. > Here you know exact type, it is just too long. Creating alias with using feels > more appropriate in this case) > Done. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:69: const FrameInfo& frame_info = frame_history_.find(frame_key)->second; On 2016/09/12 12:20:14, danilchap wrote: > frame_history_ can through out frames when there are too many of them. > Can that lead to situation where you have key in frames_ but not in > frame_history_? > i.e. may be add a DCHECK or check frame_key is in the history. Done, rewrote FrameBuffer to only have one map for frames. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:166: while (frame_history_.size() > kMaxNumHistoryFrames || On 2016/09/12 12:20:14, danilchap wrote: > probably cleaner to have two loops like before. > The erase code is small enough not to worry about code duplication. Clear logic moved to new function ClearOldInfo. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:174: auto info = frame_history_.insert(std::make_pair(key, FrameInfo())).first; On 2016/09/12 12:20:14, danilchap wrote: > since your ignore .second anyway, may be cleaner > FrameInfo* info = &frame_history_[key]; I can't rely on the pointers being stable since I insert items into the map as I iterate over it. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:185: if (ref_info->second.decoded) { On 2016/09/12 12:20:14, danilchap wrote: > do you distinguish decoded (returned by NextFrame) and decodable (has enough > packet to decode a frame, enough packet for NextFrame to return it)? > what does continuous mean? decodable? Since a FrameInfo can be inserted without the corresponding frame actually being inserted into the FrameBuffer we have to separate the case where num_missing_continuous is zero because we don't have the frame and the case where num_missing_continuous is zero due to all dependencies being fulfilled. With the new changes FrameInfo::decoded wont be needed any more. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:104: bool operator()(const FrameKey& f1, const FrameKey& f2) const { On 2016/09/12 12:20:14, danilchap wrote: > If you add this operator as a bool operator<(const FrameKey& other) const into > FrameKey > you will have one helper structure less. Done. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:105: if (f1.picture_id == f2.picture_id) On 2016/09/12 12:20:14, danilchap wrote: > what is the reason for moving implementation into .h from .cc? The idea was to make it clearer how |frames_| and |frame_history_| works by just reading the .h file. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:378: CheckLastContinuousFrame(0, pid); On 2016/09/12 12:20:14, danilchap wrote: > May be make InsertFrame return the result of buffer_.InsertFrame and merge > InsertFrame & CheckLastContinuousFrame calls. Less fixture state should make > more clear what this 5 calls refer to: > EXPECT_EQ(static_cast<int>(pid), InsertFrame(pid, 0, ts, false)); > EXPECT_EQ(static_cast<int>(pid), InsertFrame(pid + 2, 0, ts, false, pid + 1); > ... Done. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:383: } On 2016/09/12 12:20:14, danilchap wrote: > May be add a test case where InsertFrame return -1 Done.
Looks useful! While reading added few suggestions that may improve readability. https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:105: if (f1.picture_id == f2.picture_id) On 2016/09/16 14:01:20, philipel wrote: > On 2016/09/12 12:20:14, danilchap wrote: > > what is the reason for moving implementation into .h from .cc? > > The idea was to make it clearer how |frames_| and |frame_history_| works by just > reading the .h file. Acknowledged. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:37: : frames_(), may be do not mention member if you create it with default constructor. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:70: // acquire the look unnecesserily. s/look/lock/ https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:76: if (last_decoded_frame_it_ == frames_.end()) { you use this construction twice, may be add a small helper methods: iterator FirstUndecoded() const LOCK_REQUIRED; https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:89: while (frame_it != continuous_end_it) { since you ++frame_it before both continue, may be turn while into for loop: for (;frame_it != continuous_end_it; ++frame_it) { } https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:120: if (!new_countinuous_frame_event_.Wait(wait_ms)) { though unrelated to this cl, may be cleaner to turn while(true) loop into do { } while (new_continous_frame_event.Wait(wait_ms)); ...Same post wait code that return kTimeout or kFrameFound https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:198: auto info = UpdateFrameInfo(*frame.get()); no need to .get(): UpdateFrameInfo(*frame); should work. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:201: LOG(LS_INFO) << "Frame with (picture_id:spatial_id) (" << key.picture_id may be move this log inside UpdateFrameInfo, closer to the code that explain why info == frames_.end() means unresolvable dependency. Here it uses hidden knowledge that there is no other reason for UpdateFrameInfo to fail. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:230: std::queue<FrameMap::iterator> continuous_frames_; remove last '_' in the variable name. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:266: ++last_decoded_frame_it_; May be add DCHECK(last_decoded_frame_it_->first < decoded->first) Without it one can wonder 'what would happen if last_decoded_frame_it_ points to the last [not end] element in the map'. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:268: --num_frames_buffered_; probably join this with another lonely ++num_frames_history; move to top and add a comment that |decoded| changed it state from 'buffered' to 'history' https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:271: while (last_decoded_frame_it_ != decoded) { may be do not use this variable to iterate over undecoded frames, but rather introduce a new one, just for better name: non_decoded or next_frame or frame. loop while (frame != decoded) would look more natural then. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:301: (ref_key < last_decoded_frame_it_->first || may be !(last_decoded_frame_it_->first < ref_key) or prefer to have extra operator for clarity here?: (ref_key <= last_decoded_frame_it_->first) https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:303: // Do we depend on a frame that was never decoded? s/Do we/Does |frame|/ https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:330: FrameKey ref_key = FrameKey(frame.picture_id, frame.spatial_layer - 1); FrameKey ref_key(frame.picture_id, frame.spatial_layer - 1); https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:331: auto ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first; this ref_info is not used outside as iterator, may be FrameInfo* ref_info = &frames_[ref_key]; if (ref_info->continnuous)... ref_info->dependent_frames[ref_info->num_dependent_frames] = key; ++ref_info->num_denepdent_frames; https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:84: bool operator==(const FrameKey& other) const { look like instead you want bool operator<=(const FrameKey& other) const { return !(other < *this); } https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:100: // How many unfulfilled frames this frame have to become continuous. may be add description what is continuous and what is decodable frames. I failed to understand it reading just .h and _unittest.cc Had to check .cc https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:117: void PropagateContinuity(const FrameMap::iterator& start) probably better to treat iterator as pointer and pass it by value. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:128: FrameMap::iterator UpdateFrameInfo(const FrameObject& frame) this name doesn't describe neither how it will be updated, nor when. May be capture either of this in function name: OnNewFrame CreateOrGetFrameInfo CalculateDependencies https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:133: void AdvanceLastDecodedFrame(const FrameMap::iterator& decoded) pass iterator by value https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:378: Likely need a test or few with inter_layer_predicted = true which imply some tests with spatial_layer > 0
https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:37: : frames_(), On 2016/09/19 12:21:50, danilchap wrote: > may be do not mention member if you create it with default constructor. Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:70: // acquire the look unnecesserily. On 2016/09/19 12:21:50, danilchap wrote: > s/look/lock/ Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:76: if (last_decoded_frame_it_ == frames_.end()) { On 2016/09/19 12:21:50, danilchap wrote: > you use this construction twice, may be add a small helper methods: > iterator FirstUndecoded() const LOCK_REQUIRED; I guess you are thinking of line 263? It's similar but not the same. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:89: while (frame_it != continuous_end_it) { On 2016/09/19 12:21:50, danilchap wrote: > since you ++frame_it before both continue, may be turn while into for loop: > for (;frame_it != continuous_end_it; ++frame_it) { > } Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:120: if (!new_countinuous_frame_event_.Wait(wait_ms)) { On 2016/09/19 12:21:50, danilchap wrote: > though unrelated to this cl, may be cleaner to turn while(true) loop into > do { > } while (new_continous_frame_event.Wait(wait_ms)); > > ...Same post wait code that return kTimeout or kFrameFound Much cleaner, thank you! https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:198: auto info = UpdateFrameInfo(*frame.get()); On 2016/09/19 12:21:50, danilchap wrote: > no need to .get(): > UpdateFrameInfo(*frame); > should work. Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:201: LOG(LS_INFO) << "Frame with (picture_id:spatial_id) (" << key.picture_id On 2016/09/19 12:21:50, danilchap wrote: > may be move this log inside UpdateFrameInfo, closer to the code that explain why > info == frames_.end() means unresolvable dependency. > > Here it uses hidden knowledge that there is no other reason for UpdateFrameInfo > to fail. UpdateFrameInfoWithIncomingFrame now returns false if |frame| will never become continuous. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:230: std::queue<FrameMap::iterator> continuous_frames_; On 2016/09/19 12:21:50, danilchap wrote: > remove last '_' in the variable name. Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:266: ++last_decoded_frame_it_; On 2016/09/19 12:21:50, danilchap wrote: > May be add DCHECK(last_decoded_frame_it_->first < decoded->first) > Without it one can wonder 'what would happen if last_decoded_frame_it_ points to > the last [not end] element in the map'. Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:268: --num_frames_buffered_; On 2016/09/19 12:21:50, danilchap wrote: > probably join this with another lonely > ++num_frames_history; > move to top and add a comment that |decoded| changed it state from 'buffered' to > 'history' Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:271: while (last_decoded_frame_it_ != decoded) { On 2016/09/19 12:21:50, danilchap wrote: > may be do not use this variable to iterate over undecoded frames, but rather > introduce a new one, just for better name: > non_decoded or next_frame or frame. > loop while (frame != decoded) would look more natural then. I don't think this is unexpected considering the function name AdvanceLastDecodedFrame. Maybe AdvanceLastDecodedFrameIterator is clearer? https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:301: (ref_key < last_decoded_frame_it_->first || On 2016/09/19 12:21:50, danilchap wrote: > may be > !(last_decoded_frame_it_->first < ref_key) > or prefer to have extra operator for clarity here?: > (ref_key <= last_decoded_frame_it_->first) Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:303: // Do we depend on a frame that was never decoded? On 2016/09/19 12:21:50, danilchap wrote: > s/Do we/Does |frame|/ Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:330: FrameKey ref_key = FrameKey(frame.picture_id, frame.spatial_layer - 1); On 2016/09/19 12:21:50, danilchap wrote: > FrameKey ref_key(frame.picture_id, frame.spatial_layer - 1); Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:331: auto ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first; On 2016/09/19 12:21:50, danilchap wrote: > this ref_info is not used outside as iterator, may be > FrameInfo* ref_info = &frames_[ref_key]; > if (ref_info->continnuous)... > ref_info->dependent_frames[ref_info->num_dependent_frames] = key; > ++ref_info->num_denepdent_frames; Using an iterator is more in style with the rest of the function I think. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:84: bool operator==(const FrameKey& other) const { On 2016/09/19 12:21:50, danilchap wrote: > look like instead you want > bool operator<=(const FrameKey& other) const { > return !(other < *this); > } Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:100: // How many unfulfilled frames this frame have to become continuous. On 2016/09/19 12:21:51, danilchap wrote: > may be add description what is continuous and what is decodable frames. I failed > to understand it reading just .h and _unittest.cc > Had to check .cc Added comments. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:117: void PropagateContinuity(const FrameMap::iterator& start) On 2016/09/19 12:21:50, danilchap wrote: > probably better to treat iterator as pointer and pass it by value. Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:128: FrameMap::iterator UpdateFrameInfo(const FrameObject& frame) On 2016/09/19 12:21:50, danilchap wrote: > this name doesn't describe neither how it will be updated, nor when. May be > capture either of this in function name: > OnNewFrame > CreateOrGetFrameInfo > CalculateDependencies UpdateFrameInfoWithIncomingFrame https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:133: void AdvanceLastDecodedFrame(const FrameMap::iterator& decoded) On 2016/09/19 12:21:50, danilchap wrote: > pass iterator by value Done. https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/2322263002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:378: On 2016/09/19 12:21:51, danilchap wrote: > Likely need a test or few with inter_layer_predicted = true > which imply some tests with spatial_layer > 0 Done.
https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:53: int64_t now_ms = clock_->TimeInMilliseconds(); now_ms is not used outside do loop, so might be better to move this line between 57&58 and remove line 64. If you prefer it here, swap with previous line and use now_ms in it. Both ways you save one call to clock_ https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:251: void FrameBuffer::AdvanceLastDecodedFrame(FrameMap::iterator decoded) { Order [new] functions same as in header file, i.e. swap AdvanceLastDecoded and UpdateFrameInfo either here on in the header. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:264: --num_frames_buffered_; what is num_frames_buffered? number of frame objects or number of frame infos? if just frames, then you might decrease here too much. if number of frameinfos, then you do not increase enough when adding frameinfo without frame object. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:321: ++info->second.num_missing_decodable; no check in this block if referenced frame was already decoded? https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:119: void PropagateContinuity(const FrameMap::iterator start) remove const https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:135: void AdvanceLastDecodedFrame(const FrameMap::iterator decoded) ditto https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); the logic might be simplified if instead of keeping border line between decoded and might be decoded frames you would keep std::set<FrameKey> decoded_; (for those frames no FrameInfo member is relevant) and buffed frames_ separately. That would eat num_frames_history variable too (== decoded_.size()).
https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:53: int64_t now_ms = clock_->TimeInMilliseconds(); On 2016/09/19 17:44:48, danilchap wrote: > now_ms is not used outside do loop, so might be better to move this line between > 57&58 and remove line 64. > If you prefer it here, swap with previous line and use now_ms in it. > Both ways you save one call to clock_ Done. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:251: void FrameBuffer::AdvanceLastDecodedFrame(FrameMap::iterator decoded) { On 2016/09/19 17:44:48, danilchap wrote: > Order [new] functions same as in header file, > i.e. swap AdvanceLastDecoded and UpdateFrameInfo either here on in the header. Done. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:264: --num_frames_buffered_; On 2016/09/19 17:44:48, danilchap wrote: > what is num_frames_buffered? > number of frame objects or number of frame infos? > if just frames, then you might decrease here too much. > if number of frameinfos, then you do not increase enough when adding frameinfo > without frame object. Suppose to be number of buffered frames, fixed https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:321: ++info->second.num_missing_decodable; On 2016/09/19 17:44:48, danilchap wrote: > no check in this block if referenced frame was already decoded? No need, the DCHECK on line 280 ensures us that we only use this function for frames newer than |last_decoded_frame_it_|. That means the frame (frame.picture_id, frame.spatial_layer - 1) is either decoded and the FrameInfo already exist, or we haven't decoded that far which means it is safe to insert a FrameInfo into |frames_| to hold the backwards reference. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:119: void PropagateContinuity(const FrameMap::iterator start) On 2016/09/19 17:44:48, danilchap wrote: > remove const Done. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:135: void AdvanceLastDecodedFrame(const FrameMap::iterator decoded) On 2016/09/19 17:44:48, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/19 17:44:48, danilchap wrote: > the logic might be simplified if instead of keeping border line between decoded > and might be decoded frames you would keep > std::set<FrameKey> decoded_; (for those frames no FrameInfo member is relevant) > and buffed frames_ separately. That would eat num_frames_history variable too > (== decoded_.size()). Well, that is how it worked in PS3, and then I changed so that we only use one map to store both history and buffered frames. I think it's cleaner for two reasons: - We don't have to look in both history and buffered frames to test whether a frame is continuous or not. - We don't have to perform as many finds in the two different maps to get the information we need to insert/extract a frame.
https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:321: ++info->second.num_missing_decodable; On 2016/09/20 09:30:25, philipel wrote: > On 2016/09/19 17:44:48, danilchap wrote: > > no check in this block if referenced frame was already decoded? > > No need, the DCHECK on line 280 ensures us that we only use this function for > frames newer than |last_decoded_frame_it_|. That means the frame > (frame.picture_id, frame.spatial_layer - 1) is either decoded and the FrameInfo > already exist, or we haven't decoded that far which means it is safe to insert a > FrameInfo into |frames_| to hold the backwards reference. What about situation ref_key == last_decoded_frame_it->first ? then info->second.num_missing_decodable shouldn't be incremented. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/20 09:30:25, philipel wrote: > On 2016/09/19 17:44:48, danilchap wrote: > > the logic might be simplified if instead of keeping border line between > decoded > > and might be decoded frames you would keep > > std::set<FrameKey> decoded_; (for those frames no FrameInfo member is > relevant) > > and buffed frames_ separately. That would eat num_frames_history variable too > > (== decoded_.size()). > > Well, that is how it worked in PS3, and then I changed so that we only use one > map to store both history and buffered frames. > > I think it's cleaner for two reasons: > - We don't have to look in both history and buffered frames to test whether a > frame is continuous or not. > - We don't have to perform as many finds in the two different maps to get the > information we need to insert/extract a frame. no, PS3 did it differently than what I propose here: once frame has been decoded, i.e. returned by NextFrame, you do not need to keep FrameInfo about it: it is continous, have no missing references, FrameObject is gone and since PropogateDecodability just called, back references are not needed either. All you need is FrameKey. And, sometimes, last_decoded_key = *decoded_.rbegin(); So you can add it to decoded_ and remove from frames_. Whenever you search, you treat decoded frames and not yet decoded frames differently. If found in decoded (for num_missing_decodable update), no point to search in frames_. PropagateContinuity can't find already decoded frames because they do not have any missing frames for continuity. This is just a suggestion, if you find current way cleaner, keep it.
https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:321: ++info->second.num_missing_decodable; On 2016/09/20 11:15:08, danilchap wrote: > On 2016/09/20 09:30:25, philipel wrote: > > On 2016/09/19 17:44:48, danilchap wrote: > > > no check in this block if referenced frame was already decoded? > > > > No need, the DCHECK on line 280 ensures us that we only use this function for > > frames newer than |last_decoded_frame_it_|. That means the frame > > (frame.picture_id, frame.spatial_layer - 1) is either decoded and the > FrameInfo > > already exist, or we haven't decoded that far which means it is safe to insert > a > > FrameInfo into |frames_| to hold the backwards reference. > > What about situation ref_key == last_decoded_frame_it->first ? > then info->second.num_missing_decodable shouldn't be incremented. You are absolutely right, fixed + added unittest. https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/20 11:15:08, danilchap wrote: > On 2016/09/20 09:30:25, philipel wrote: > > On 2016/09/19 17:44:48, danilchap wrote: > > > the logic might be simplified if instead of keeping border line between > > decoded > > > and might be decoded frames you would keep > > > std::set<FrameKey> decoded_; (for those frames no FrameInfo member is > > relevant) > > > and buffed frames_ separately. That would eat num_frames_history variable > too > > > (== decoded_.size()). > > > > Well, that is how it worked in PS3, and then I changed so that we only use one > > map to store both history and buffered frames. > > > > I think it's cleaner for two reasons: > > - We don't have to look in both history and buffered frames to test whether a > > frame is continuous or not. > > - We don't have to perform as many finds in the two different maps to get the > > information we need to insert/extract a frame. > > no, PS3 did it differently than what I propose here: > once frame has been decoded, i.e. returned by NextFrame, you do not need to keep > FrameInfo about it: it is continous, have no missing references, FrameObject is > gone and since PropogateDecodability just called, back references are not needed > either. All you need is FrameKey. And, sometimes, last_decoded_key = > *decoded_.rbegin(); > So you can add it to decoded_ and remove from frames_. > > Whenever you search, you treat decoded frames and not yet decoded frames > differently. If found in decoded (for num_missing_decodable update), no point to > search in frames_. > PropagateContinuity can't find already decoded frames because they do not have > any missing frames for continuity. > > This is just a suggestion, if you find current way cleaner, keep it. I'm not sure I understand your suggestion.
https://codereview.webrtc.org/2322263002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:331: ref_info->second.dependent_frames[ref_info->second.num_dependent_frames] = this one not needed if (ref_info == last_decoded_frame_it)
https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/20 11:44:44, philipel wrote: > On 2016/09/20 11:15:08, danilchap wrote: > > On 2016/09/20 09:30:25, philipel wrote: > > > On 2016/09/19 17:44:48, danilchap wrote: > > > > the logic might be simplified if instead of keeping border line between > > > decoded > > > > and might be decoded frames you would keep > > > > std::set<FrameKey> decoded_; (for those frames no FrameInfo member is > > > relevant) > > > > and buffed frames_ separately. That would eat num_frames_history variable > > too > > > > (== decoded_.size()). > > > > > > Well, that is how it worked in PS3, and then I changed so that we only use > one > > > map to store both history and buffered frames. > > > > > > I think it's cleaner for two reasons: > > > - We don't have to look in both history and buffered frames to test whether > a > > > frame is continuous or not. > > > - We don't have to perform as many finds in the two different maps to get > the > > > information we need to insert/extract a frame. > > > > no, PS3 did it differently than what I propose here: > > once frame has been decoded, i.e. returned by NextFrame, you do not need to > keep > > FrameInfo about it: it is continous, have no missing references, FrameObject > is > > gone and since PropogateDecodability just called, back references are not > needed > > either. All you need is FrameKey. And, sometimes, last_decoded_key = > > *decoded_.rbegin(); > > So you can add it to decoded_ and remove from frames_. > > > > Whenever you search, you treat decoded frames and not yet decoded frames > > differently. If found in decoded (for num_missing_decodable update), no point > to > > search in frames_. > > PropagateContinuity can't find already decoded frames because they do not have > > any missing frames for continuity. > > > > This is just a suggestion, if you find current way cleaner, keep it. > > I'm not sure I understand your suggestion. Unfortunately it required a few more changes since an iterator can't be used to describe the last continuous frame, which would have to be replaced by a FrameKey and a bool to describe if the FrameKey is valid or not, and also a few more finds. https://codereview.webrtc.org/2322263002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:331: ref_info->second.dependent_frames[ref_info->second.num_dependent_frames] = On 2016/09/20 12:06:36, danilchap wrote: > this one not needed if (ref_info == last_decoded_frame_it) Done.
lgtm https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/20 13:11:08, philipel wrote: > On 2016/09/20 11:44:44, philipel wrote: > > On 2016/09/20 11:15:08, danilchap wrote: > > > On 2016/09/20 09:30:25, philipel wrote: > > > > On 2016/09/19 17:44:48, danilchap wrote: > > > > > the logic might be simplified if instead of keeping border line between > > > > decoded > > > > > and might be decoded frames you would keep > > > > > std::set<FrameKey> decoded_; (for those frames no FrameInfo member is > > > > relevant) > > > > > and buffed frames_ separately. That would eat num_frames_history > variable > > > too > > > > > (== decoded_.size()). > > > > > > > > Well, that is how it worked in PS3, and then I changed so that we only use > > one > > > > map to store both history and buffered frames. > > > > > > > > I think it's cleaner for two reasons: > > > > - We don't have to look in both history and buffered frames to test > whether > > a > > > > frame is continuous or not. > > > > - We don't have to perform as many finds in the two different maps to get > > the > > > > information we need to insert/extract a frame. > > > > > > no, PS3 did it differently than what I propose here: > > > once frame has been decoded, i.e. returned by NextFrame, you do not need to > > keep > > > FrameInfo about it: it is continous, have no missing references, FrameObject > > is > > > gone and since PropogateDecodability just called, back references are not > > needed > > > either. All you need is FrameKey. And, sometimes, last_decoded_key = > > > *decoded_.rbegin(); > > > So you can add it to decoded_ and remove from frames_. > > > > > > Whenever you search, you treat decoded frames and not yet decoded frames > > > differently. If found in decoded (for num_missing_decodable update), no > point > > to > > > search in frames_. > > > PropagateContinuity can't find already decoded frames because they do not > have > > > any missing frames for continuity. > > > > > > This is just a suggestion, if you find current way cleaner, keep it. > > > > I'm not sure I understand your suggestion. > > Unfortunately it required a few more changes since an iterator can't be used to > describe the last continuous frame, which would have to be replaced by a > FrameKey and a bool to describe if the FrameKey is valid or not, and also a few > more finds. Hm, yes, sorry, missed scenario last_continuous_frame == last_decoded_frame. replacing last_continuous_frame with rtc::Optional<FrameKey> doesn't make code that much cleaner. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:93: int64_t render_time = may be shorten this block a bit: if (frame->RenderTime() == -1) frame->SetRenderTime(timeing_->RenderTimeMs(frame->timestamp, now_ms); wait_ms = timing_->MaxWaitingTime(frame->RenderTime(), now_ms);
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:28: constexpr int kMaxFramesBuffered = 1000; Old jitter buffer had this limited to 300. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:81: // |continuous_end_it| point to the first frame after the points https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:117: int64_t received_timestamp = frame->ReceivedTime(); received_time instead of received_timestamp? The word "timestamp" is fairly tightly coupled to the actual frame timestamp, which might be confusing. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:164: LOG(LS_INFO) << "Frame with (picture_id:spatial_id) (" << key.picture_id Should these logs be warnings? https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:165: << ":" << int(key.spatial_layer) static_cast<int>() here and below. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:200: PropagateContinuity(info); I think it would be easier to read the code if you did last_continuous_frame_it_ = PropagateContinuity(info); That way you may even be able to make PropagateContinuity() static if you pass in frames_ https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:283: // Check how many dependencies that has already been fulfilled. have already https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:284: for (size_t r = 0; r < frame.num_references; ++r) { use i instead of r. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:300: --info->second.num_missing_continuous; Does this mean the frame has already been decoded? Add a comment. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:302: Remove empty line https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:324: auto ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first; When doing things like this I think it would be good to have a comment saying "Find the reference frame or create an empty FrameInfo if it hasn't been received yet." https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:78: bool operator<(const FrameKey& other) const { I think rhs is a better name than other as it makes the side of the operator more clear. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:84: bool operator<=(const FrameKey& other) const { return !(other < *this); } Same here. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:94: FrameKey dependent_frames[kMaxNumDependentFrames]; Comment on if this array only contains direct dependencies or if it also contains indirect dependencies. If the latter I guess 5 is way too little. Maybe also mention why 5 was picked? Seems a bit low to me. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:97: // A frame is continiuous if have all its referenced/indirectly referenced continuous if it has https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:100: // How many unfulfilled frames this frame have to become continuous. s/have/need? https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:101: size_t num_missing_continuous = 0; num_missing_dependencies? https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:106: size_t num_missing_decodable = 0; Not clear to me what the difference is between decodable and continuous https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:109: bool continuous = false; Isn't this true whenever num_missing_continuous is 0? https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:122: // Mark the frame as decoded and updates all directly dependent frames. "and update" https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:126: // Advances |last_decoded_frame_it_| to |decoded| and remove old "and removes" https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:127: // frame info. FrameInfo https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); Does this mean we're always keeping the last decoded frame around until another frame is decoded?
https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:28: constexpr int kMaxFramesBuffered = 1000; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Old jitter buffer had this limited to 300. I think that is a bit low, at 60 fps that is 5 seconds, changed it to 600. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:81: // |continuous_end_it| point to the first frame after the On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > points Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:93: int64_t render_time = On 2016/09/20 13:45:25, danilchap wrote: > may be shorten this block a bit: > if (frame->RenderTime() == -1) > frame->SetRenderTime(timeing_->RenderTimeMs(frame->timestamp, now_ms); > wait_ms = timing_->MaxWaitingTime(frame->RenderTime(), now_ms); Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:117: int64_t received_timestamp = frame->ReceivedTime(); On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > received_time instead of received_timestamp? The word "timestamp" is fairly > tightly coupled to the actual frame timestamp, which might be confusing. Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:164: LOG(LS_INFO) << "Frame with (picture_id:spatial_id) (" << key.picture_id On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > Should these logs be warnings? I think warnings are better since we don't expect this to be a part of normal usage of the FrameBuffer, changed. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:165: << ":" << int(key.spatial_layer) On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > static_cast<int>() here and below. Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:200: PropagateContinuity(info); On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > I think it would be easier to read the code if you did > > last_continuous_frame_it_ = PropagateContinuity(info); > > That way you may even be able to make PropagateContinuity() static if you pass > in frames_ If a new frame became continuous then |last_continuous_frame_it_| might be updated, so if we return an iterator we wont know if the new frames became continuous or not. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:283: // Check how many dependencies that has already been fulfilled. On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > have already Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:284: for (size_t r = 0; r < frame.num_references; ++r) { On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > use i instead of r. Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:300: --info->second.num_missing_continuous; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Does this mean the frame has already been decoded? Add a comment. Yes, since this |ref_key| refers to a frame more previous than |last_decoded_frame_it_| and |ref_key| exist, it means that this frame has already been decoded which also means that it is continuous. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:302: On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > Remove empty line Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:324: auto ref_info = frames_.insert(std::make_pair(ref_key, FrameInfo())).first; On 2016/09/21 14:50:13, stefan-webrtc (holmer) wrote: > When doing things like this I think it would be good to have a comment saying > "Find the reference frame or create an empty FrameInfo if it hasn't been > received yet." Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:78: bool operator<(const FrameKey& other) const { On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > I think rhs is a better name than other as it makes the side of the operator > more clear. Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:84: bool operator<=(const FrameKey& other) const { return !(other < *this); } On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Same here. Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:94: FrameKey dependent_frames[kMaxNumDependentFrames]; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Comment on if this array only contains direct dependencies or if it also > contains indirect dependencies. If the latter I guess 5 is way too little. Maybe > also mention why 5 was picked? Seems a bit low to me. Updated comment, I think 5 should be enough but I increased it to 8. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:97: // A frame is continiuous if have all its referenced/indirectly referenced On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > continuous if it has Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:100: // How many unfulfilled frames this frame have to become continuous. On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > s/have/need? this frame have until it becomes https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:101: size_t num_missing_continuous = 0; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > num_missing_dependencies? Not really, this hold how many referenced frames we are missing for this frame to become continuous. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:106: size_t num_missing_decodable = 0; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Not clear to me what the difference is between decodable and continuous A frame is continuous if all frames this frame depend, either directly or indirectly, has been received, or in other words, if we decode all of its indireclty/directly referenced frames then this frame will become decodable. A frame is decodable if it can be sent to the decoder now. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:109: bool continuous = false; On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Isn't this true whenever num_missing_continuous is 0? No, since we create FrameInfos to hold backwards references for frames we have not yet received it can be 0 even if it is not continuous. I guess this code be encoded as |num_missing_continuous| = -1 instead. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:122: // Mark the frame as decoded and updates all directly dependent frames. On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > "and update" Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:126: // Advances |last_decoded_frame_it_| to |decoded| and remove old On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > "and removes" Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:127: // frame info. On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > FrameInfo Done. https://codereview.webrtc.org/2322263002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:146: FrameMap::iterator last_decoded_frame_it_ GUARDED_BY(crit_); On 2016/09/21 14:50:14, stefan-webrtc (holmer) wrote: > Does this mean we're always keeping the last decoded frame around until another > frame is decoded? No, the actual FrameObject is passed to the decoder, but the FrameInfo remains and this iterator keeps track of which frame that was.
ping
lgtm https://codereview.webrtc.org/2322263002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2322263002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:322: // Gets or create the FrameInfo for the referenced frame. Get or create
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2322263002/#ps160001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Frame continuity is now tested as soon as a frame is inserted into the FrameBuffer. Since we want to stop sending NACKS for frames not needed to keep the stream decodable we must know which frames that are continuous or not. BUG=webrtc:5514 ========== to ========== Frame continuity is now tested as soon as a frame is inserted into the FrameBuffer. Since we want to stop sending NACKS for frames not needed to keep the stream decodable we must know which frames that are continuous or not. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/e0b2f154170ae2bfc11ddfede3241101748dfec3 Cr-Commit-Position: refs/heads/master@{#14412} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e0b2f154170ae2bfc11ddfede3241101748dfec3 Cr-Commit-Position: refs/heads/master@{#14412}
Message was sent while issue was closed.
Description was changed from ========== Frame continuity is now tested as soon as a frame is inserted into the FrameBuffer. Since we want to stop sending NACKS for frames not needed to keep the stream decodable we must know which frames that are continuous or not. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/e0b2f154170ae2bfc11ddfede3241101748dfec3 Cr-Commit-Position: refs/heads/master@{#14412} ========== to ========== Frame continuity is now tested as soon as a frame is inserted into the FrameBuffer. Since we want to stop sending NACKS for frames not needed to keep the stream decodable we must know which frames that are continuous or not. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e0b2f154170ae2bfc11ddfede... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as e0b2f154170ae2bfc11ddfede3241101748dfec3 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
