Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1969403007:
FrameBuffer for the new jitter buffer. (Closed)
|
Created:
4 years, 7 months ago by philipel Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFrameBuffer for the new jitter buffer.
BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org
Committed: https://crrev.com/a376e70cf9d0df3c35d53533b454da542661775b
Cr-Commit-Position: refs/heads/master@{#12798}
Patch Set 1 #Patch Set 2 : Added comments. #
Total comments: 57
Patch Set 3 : Added unittest for late frame insertions. #
Total comments: 17
Patch Set 4 : Feedback fixes. #Patch Set 5 : Feedback fixes. #
Total comments: 21
Patch Set 6 : Implemented FrameKey and changed decoded_frames_ to a std::set<FrameKey>, also feedback fixes. #
Total comments: 18
Patch Set 7 : Unittest fix and feedback fixes. #
Total comments: 4
Patch Set 8 : Feedback fixes. #
Total comments: 1
Patch Set 9 : Added comment and rebased. #
Messages
Total messages: 25 (6 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, mflodman@webrtc.org
some style-like suggestions https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:13: #include "webrtc/modules/video_coding/frame_buffer2.h" this include goes first, before c++ header https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:34: auto frame_key = std::make_pair(-1, -1); frame_key has type std::pair<int, int>, but used in map where key_type is std::pair<uint16_t, uint8_t>. Probably it is better to be more explicit about the frame_key type. e.g. in .h add using FrameKey = std::pair<uint16_t, uint8_t> and use that in map definition (std::map<FrameKey, std::unique_ptr<FrameObject>, ....>, in the FrameComp struct and here, as type for frame_key. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:37: crit_.Enter(); may be use { rtc::CritScope lock(&crit_); ... } instead of crit_.Engter(); ... crit_.Leave(); Though it adds an indentation, it make clear how long you hold the lock. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:39: for (auto& kvp : frames_) { const auto& though may be explicit type (const std::pair<FrameKey, std::unique_ptr<FrameObject>>& ) would be more readable: frame_ type is far away in another file. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:40: if (IsContinuous(*(kvp.second.get()))) { IsContinuous(*kvp.second) Or add const FrameObject& frame = *kvp.second; as first line in the loop since frame is used more than once. - that is also alternative way to be more explicit about kvp type. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:43: clock_->TimeInMilliseconds()); May be you want to cache current time, or do you expect RenderTimeMs work so long that you really want to take current time twice? https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:63: crit_.Enter(); rtc::CritScope lock(&crit_); https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:68: auto frame_it = frames_.find(frame_key); since you already silently expect frame_it != frames_.end(), may be you want to save frame_it instead of frame_key. (inserts into map do not invalidate existent iterators) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:22: #include "webrtc/modules/video_coding/frame_object.h" using forward declaration allows you to move 4 includes into .cc making header lighter. namespace webrtc { class Clock; class FrameObject; class VCMJitterEstimator; class VCMTiming; namespace video_coding { https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:48: static const int kMaxFrameAge = 4096; static constexpr encouraged instead of static const (same for next 2 constants) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:68: struct FrameComp { typedefs and alike should be at the begining of the private section, before static constants. (https://google.github.io/styleguide/cppguide.html#Declaration_Order) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:70: const std::pair<uint16_t, uint8_t>& f2) const { consider moving implementation into .cc https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:16: #include "webrtc/modules/video_coding/frame_buffer2.h" put this include before c++ includes https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:20: #include "webrtc/base/thread.h" base/thread.h is not in rtc_base_approved - do not include it in modules/ (specially that you do not use it: same kForever declared in the Event that make more sense to use since you are waiting for Events) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:21: #include "testing/gmock/include/gmock/gmock.h" alphabetical order (testing before webrtc) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:29: class VCMTimingMock : public VCMTiming { this object look like Fake, not Mock https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:55: private: https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:56: static const int kDelayMs = 50; constexpr https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:100: template <typename... T> do you really want to use variable number of parameters with variable types? May be instead you want to take rtc::ArrayView<uint16_t> as last parameter. While it make InsertFrame calls two-lines instead of one, it might be clear: const uint16_t kReferences2[] = {pid + i, pid + i - 1}; InsertFrame(pid + i + 1, 0, ts_tl0 + kFps20, false, kReferences2); https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:122: crit_.Enter(); do you need to check max_wait_time while in crit_? Guess not. You can then enter/leave crit_ with CritScope (and extra {} in else branch) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:131: crit_acquired_event_.Wait(rtc::Thread::kForever); Wait(rtc::Event::kForever) https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:135: void CheckFrame(size_t index, look like you want two functions: CheckNoFrame with with one parameter, and CheckFrame with three. Instead of a branch inside the test. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:139: if (frames_.size() <= index) { ASSERT_LT(index, frames_.size()) << "Error message"; https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:148: ASSERT_EQ(nullptr, frames_[index].get()); ASSERT_FALSE(frames_[index]) will do the same, but better match assert in else branch https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:156: static bool ExtractLoop(void* obj) { may be create non-static version bool ExtractLoop(); and call that one from this static function. implementation would look nice then. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:160: tfb->crit_.Enter(); rtc::CritScope lock(&tfb->crit_); https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:172: uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint32_t>::max()); } uint16_t or uint32? btw, you can run rand_.Rand<uint32_t>(); instead of explicitly specifying max. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:174: static const int kMaxReferences = 5; constexpr and may be move above functions https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:36: uint32_t timestamp; do not forget to initialize this new member in constructor
few more suggestions. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:61: if (!frame_inserted_event_.Wait(wait_ms)) { because of the while(true) loop look like you can call this Wait multiple times and as a result do not return longer than max_wait_time_ms. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:107: // decoded frames picture id then that frame arrived to late. too late https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:43: std::unique_ptr<FrameObject> NextFrame(int64_t max_wait_time); max_wait_time_ms https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:69: bool operator()(const std::pair<uint16_t, uint8_t>& f1, probably better make this static if you want to keep it. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:81: std::map<std::pair<uint16_t, uint8_t>, may be instead of using pair as key and creating special comparer it would be clearer to create special key structure: struct FrameId { FrameId(uint16_t picture_id, uint8_t spatial_layer); uint16_t picture_id; uint8_t spatial_layer; bool operator<(const FrameId& other) const; }; https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:86: Clock* clock_; Clock* const clock_; https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:88: VCMJitterEstimator* jitter_estimator_; VCMJitterEstimator* const jitter_estimator_; https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:89: VCMTiming* timing_; const VCMTiming* const timing_;
https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:13: #include "webrtc/modules/video_coding/frame_buffer2.h" On 2016/05/13 13:31:04, danilchap wrote: > this include goes first, before c++ header Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:34: auto frame_key = std::make_pair(-1, -1); On 2016/05/13 13:31:04, danilchap wrote: > frame_key has type std::pair<int, int>, but used in map where key_type is > std::pair<uint16_t, uint8_t>. > Probably it is better to be more explicit about the frame_key type. > e.g. in .h add > using FrameKey = std::pair<uint16_t, uint8_t> > and use that in map definition (std::map<FrameKey, std::unique_ptr<FrameObject>, > ....>, > in the FrameComp struct and here, as type for frame_key. I no longer use a pair to keep track of which frame to return. I now use an iterator instead. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:37: crit_.Enter(); On 2016/05/13 13:31:04, danilchap wrote: > may be use > { > rtc::CritScope lock(&crit_); > ... > } > > instead of > crit_.Engter(); > ... > crit_.Leave(); > > > Though it adds an indentation, it make clear how long you hold the lock. After the change to use an iterator instead of a frame key there will be problems with the scope of variables if i try to use rtc::CritScope. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:40: if (IsContinuous(*(kvp.second.get()))) { On 2016/05/13 13:31:04, danilchap wrote: > IsContinuous(*kvp.second) > Or add > const FrameObject& frame = *kvp.second; > as first line in the loop since frame is used more than once. - that is also > alternative way to be more explicit about kvp type. Added an additional line. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:43: clock_->TimeInMilliseconds()); On 2016/05/13 13:31:04, danilchap wrote: > May be you want to cache current time, > or do you expect RenderTimeMs work so long that you really want to take current > time twice? Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:63: crit_.Enter(); On 2016/05/13 13:31:04, danilchap wrote: > rtc::CritScope lock(&crit_); Don't want to use two different styles within the same function. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:68: auto frame_it = frames_.find(frame_key); On 2016/05/13 13:31:04, danilchap wrote: > since you already silently expect frame_it != frames_.end(), > may be you want to save frame_it instead of frame_key. > (inserts into map do not invalidate existent iterators) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:22: #include "webrtc/modules/video_coding/frame_object.h" On 2016/05/13 13:31:04, danilchap wrote: > using forward declaration allows you to move 4 includes into .cc making header > lighter. > > namespace webrtc { > class Clock; > class FrameObject; > class VCMJitterEstimator; > class VCMTiming; > > namespace video_coding { Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:48: static const int kMaxFrameAge = 4096; On 2016/05/13 13:31:04, danilchap wrote: > static constexpr encouraged instead of static const (same for next 2 constants) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:68: struct FrameComp { On 2016/05/13 13:31:04, danilchap wrote: > typedefs and alike should be at the begining of the private section, before > static constants. > (https://google.github.io/styleguide/cppguide.html#Declaration_Order) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:70: const std::pair<uint16_t, uint8_t>& f2) const { On 2016/05/13 13:31:04, danilchap wrote: > consider moving implementation into .cc Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:20: #include "webrtc/base/thread.h" On 2016/05/13 13:31:04, danilchap wrote: > base/thread.h is not in rtc_base_approved - do not include it in modules/ > (specially that you do not use it: same kForever declared in the Event that make > more sense to use since you are waiting for Events) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:21: #include "testing/gmock/include/gmock/gmock.h" On 2016/05/13 13:31:04, danilchap wrote: > alphabetical order (testing before webrtc) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:29: class VCMTimingMock : public VCMTiming { On 2016/05/13 13:31:04, danilchap wrote: > this object look like Fake, not Mock Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:55: On 2016/05/13 13:31:04, danilchap wrote: > private: Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:56: static const int kDelayMs = 50; On 2016/05/13 13:31:04, danilchap wrote: > constexpr Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:100: template <typename... T> On 2016/05/13 13:31:04, danilchap wrote: > do you really want to use variable number of parameters with variable types? > May be instead you want to take rtc::ArrayView<uint16_t> as last parameter. > > While it make InsertFrame calls two-lines instead of one, it might be clear: > const uint16_t kReferences2[] = {pid + i, pid + i - 1}; > InsertFrame(pid + i + 1, 0, ts_tl0 + kFps20, false, kReferences2); Prefer variadic function. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:122: crit_.Enter(); On 2016/05/13 13:31:04, danilchap wrote: > do you need to check max_wait_time while in crit_? Guess not. > You can then enter/leave crit_ with CritScope (and extra {} in else branch) We always want to hold |crit_| in the beginning of this function in order to sync with other events. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:131: crit_acquired_event_.Wait(rtc::Thread::kForever); On 2016/05/13 13:31:05, danilchap wrote: > Wait(rtc::Event::kForever) Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:139: if (frames_.size() <= index) { On 2016/05/13 13:31:05, danilchap wrote: > ASSERT_LT(index, frames_.size()) << "Error message"; Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:148: ASSERT_EQ(nullptr, frames_[index].get()); On 2016/05/13 13:31:05, danilchap wrote: > ASSERT_FALSE(frames_[index]) will do the same, but better match assert in else > branch Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:156: static bool ExtractLoop(void* obj) { On 2016/05/13 13:31:05, danilchap wrote: > may be create non-static version bool ExtractLoop(); > and call that one from this static function. > implementation would look nice then. Acknowledged. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:160: tfb->crit_.Enter(); On 2016/05/13 13:31:05, danilchap wrote: > rtc::CritScope lock(&tfb->crit_); Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:172: uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint32_t>::max()); } On 2016/05/13 13:31:05, danilchap wrote: > uint16_t or uint32? > > btw, you can run rand_.Rand<uint32_t>(); instead of explicitly specifying max. Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:174: static const int kMaxReferences = 5; On 2016/05/13 13:31:05, danilchap wrote: > constexpr and may be move above functions Done. https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:36: uint32_t timestamp; On 2016/05/13 13:31:05, danilchap wrote: > do not forget to initialize this new member in constructor Done.
https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:61: if (!frame_inserted_event_.Wait(wait_ms)) { On 2016/05/13 15:33:56, danilchap wrote: > because of the while(true) loop look like you can call this Wait multiple times > and as a result do not return longer than max_wait_time_ms. Fixed. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:43: std::unique_ptr<FrameObject> NextFrame(int64_t max_wait_time); On 2016/05/13 15:33:56, danilchap wrote: > max_wait_time_ms Done. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:69: bool operator()(const std::pair<uint16_t, uint8_t>& f1, On 2016/05/13 15:33:56, danilchap wrote: > probably better make this static if you want to keep it. I don't really understand... https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:81: std::map<std::pair<uint16_t, uint8_t>, On 2016/05/13 15:33:56, danilchap wrote: > may be instead of using pair as key and creating special comparer it would be > clearer to create special key structure: > struct FrameId { > FrameId(uint16_t picture_id, uint8_t spatial_layer); > uint16_t picture_id; > uint8_t spatial_layer; > bool operator<(const FrameId& other) const; > }; Acknowledged. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:86: Clock* clock_; On 2016/05/13 15:33:56, danilchap wrote: > Clock* const clock_; Done. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:88: VCMJitterEstimator* jitter_estimator_; On 2016/05/13 15:33:57, danilchap wrote: > VCMJitterEstimator* const jitter_estimator_; Done. https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:89: VCMTiming* timing_; On 2016/05/13 15:33:56, danilchap wrote: > const VCMTiming* const timing_; Done.
https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:135: void CheckFrame(size_t index, On 2016/05/13 13:31:04, danilchap wrote: > look like you want two functions: CheckNoFrame with with one parameter, and > CheckFrame with three. Instead of a branch inside the test. overlooked or disagreed? https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:69: bool operator()(const std::pair<uint16_t, uint8_t>& f1, On 2016/05/17 09:16:41, philipel wrote: > On 2016/05/13 15:33:56, danilchap wrote: > > probably better make this static if you want to keep it. > > I don't really understand... i.e. static bool operator()(<same as before>) const; to help optimizer and tools. though I do not know any case where it would matter. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:45: int64_t wait_ms = max_wait_time_ms; probably better to put inside while(true) loop. Otherwise it look like you might reuse wait_ms from previous search for frame. Even if it is not possible or not problematic, that is not obvious. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:58: // frame_key = kvp.first; can be removed? https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:76: wait_ms = std::max(wait_ms, 0l); if plain 0 doesn't work, you might want to write 0ll (i.e. long long) = always 64bit instead of l = long that is 32bit on 32bit system) https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:80: // TODO(philipel): update jitter estimator with correct values. this TODO is subject for follow-up CL or you can resolve it here? https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:90: break; may be move return here from end of the function, making more visible where this break jump out. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:107: while (!decoded_frames_.empty() && may be split it into two loops, i.e. first remove while (decoded_frames_.size() > kMaxStoredFrames) then remove while (!empty() && ForwardDiff()) to make condition(s) simpler https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:38: VCMTiming* timing); Frame buffer uses only const functions from timing, so may be const VCMTiming* timing https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:44: // |max_wait_time|, with either a managed FrameObject or an empty |max_wait_time_ms| https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:60: // The maximum number of frames stored in the frame buffer. This comment is misleading: constant is used to limit history of returned frames (decoded_frames_) instead of limiting number of stored frames (frames_) May be name it kMaxHistoryDecodedFrames https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:72: std::map<uint16_t, de-facto decoded_frames_ use same key as frames_. May be reflect it in code by using same std::pair? (but then std::map<key, bool> where bool can only be true would be de-facto std::set<key>) or you want to keep this kind of structure to keep track of MaxStoredDifferentFrames instead of MaxStoredFrames * UsedSpatialLayers?
https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:135: void CheckFrame(size_t index, On 2016/05/17 12:35:01, danilchap wrote: > On 2016/05/13 13:31:04, danilchap wrote: > > look like you want two functions: CheckNoFrame with with one parameter, and > > CheckFrame with three. Instead of a branch inside the test. > > overlooked or disagreed? Fixed now :) https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:69: bool operator()(const std::pair<uint16_t, uint8_t>& f1, On 2016/05/17 12:35:02, danilchap wrote: > On 2016/05/17 09:16:41, philipel wrote: > > On 2016/05/13 15:33:56, danilchap wrote: > > > probably better make this static if you want to keep it. > > > > I don't really understand... > > i.e. static bool operator()(<same as before>) const; > to help optimizer and tools. though I do not know any case where it would > matter. Acknowledged. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:45: int64_t wait_ms = max_wait_time_ms; On 2016/05/17 12:35:02, danilchap wrote: > probably better to put inside while(true) loop. > Otherwise it look like you might reuse wait_ms from previous search for frame. > Even if it is not possible or not problematic, that is not obvious. Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:58: // frame_key = kvp.first; On 2016/05/17 12:35:02, danilchap wrote: > can be removed? Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:76: wait_ms = std::max(wait_ms, 0l); On 2016/05/17 12:35:02, danilchap wrote: > if plain 0 doesn't work, you might want to write 0ll (i.e. long long) = always > 64bit instead of l = long that is 32bit on 32bit system) Changed to min<int64_t>/max<int64_t> https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:80: // TODO(philipel): update jitter estimator with correct values. On 2016/05/17 12:35:02, danilchap wrote: > this TODO is subject for follow-up CL or you can resolve it here? Have to wait for a follow-up CL. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:90: break; On 2016/05/17 12:35:02, danilchap wrote: > may be move return here from end of the function, making more visible where this > break jump out. Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:107: while (!decoded_frames_.empty() && On 2016/05/17 12:35:02, danilchap wrote: > may be split it into two loops, i.e. first remove while (decoded_frames_.size() > > kMaxStoredFrames) > then remove while (!empty() && ForwardDiff()) to make condition(s) simpler Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:38: VCMTiming* timing); On 2016/05/17 12:35:02, danilchap wrote: > Frame buffer uses only const functions from timing, so may be const VCMTiming* > timing Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:44: // |max_wait_time|, with either a managed FrameObject or an empty On 2016/05/17 12:35:02, danilchap wrote: > |max_wait_time_ms| Done. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:60: // The maximum number of frames stored in the frame buffer. On 2016/05/17 12:35:02, danilchap wrote: > This comment is misleading: constant is used to limit history of returned frames > (decoded_frames_) instead of limiting number of stored frames (frames_) > May be name it kMaxHistoryDecodedFrames Changed to kMaxHistoryDecodedFrames and updated comment. https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.h:72: std::map<uint16_t, On 2016/05/17 12:35:02, danilchap wrote: > de-facto decoded_frames_ use same key as frames_. May be reflect it in code by > using same std::pair? (but then std::map<key, bool> where bool can only be true > would be de-facto std::set<key>) > or you want to keep this kind of structure to keep track of > MaxStoredDifferentFrames instead of MaxStoredFrames * UsedSpatialLayers? Changed to a std::set instead.
https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:76: wait_ms = std::max(wait_ms, 0l); On 2016/05/17 14:57:04, philipel wrote: > On 2016/05/17 12:35:02, danilchap wrote: > > if plain 0 doesn't work, you might want to write 0ll (i.e. long long) = always > > 64bit instead of l = long that is 32bit on 32bit system) > > Changed to min<int64_t>/max<int64_t> nice! https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:26: // The maximum age of a frame stored in the frame buffer, compared to 'decoded frames tracked by' instead of 'frame stored in', like in next comment https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:28: static constexpr int kMaxFrameAge = 4096; since this constants are not inside class now, static is not needed. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:95: decoded_frames_.insert( if you move this lines a bit above, you can write decoded_frames_.insert(next_frame->first); https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:115: // Remove frames as long as we have to many, |kMaxNumHistoryFrames|. too many https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); old_picture_id is oldest allowed picture_id (it's age is exactly MaxAge), so (old_picture_id, 0) shouldn't be erased. Replacing kMaxSpatialLayers with 0 should fix this issue and allows to remove kMaxSpatialLayers constant completely. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:146: FrameKey ref_key(frame.picture_id, frame.spatial_layer); frame.spatial_layer - 1 https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:62: // Keep track of which decoded franes. Keep track of decoded frames?
https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:26: // The maximum age of a frame stored in the frame buffer, compared to On 2016/05/17 16:21:39, danilchap wrote: > 'decoded frames tracked by' instead of 'frame stored in', like in next comment Done. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:28: static constexpr int kMaxFrameAge = 4096; On 2016/05/17 16:21:38, danilchap wrote: > since this constants are not inside class now, static is not needed. Done. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:95: decoded_frames_.insert( On 2016/05/17 16:21:38, danilchap wrote: > if you move this lines a bit above, you can write > decoded_frames_.insert(next_frame->first); Nice, didn't think of that. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:115: // Remove frames as long as we have to many, |kMaxNumHistoryFrames|. On 2016/05/17 16:21:38, danilchap wrote: > too many Done. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); On 2016/05/17 16:21:38, danilchap wrote: > old_picture_id is oldest allowed picture_id (it's age is exactly MaxAge), so > (old_picture_id, 0) shouldn't be erased. > Replacing kMaxSpatialLayers with 0 should fix this issue and allows to remove > kMaxSpatialLayers constant completely. erase removes the elements in the range [begin, old_decoded_it), so the element old_decoded_it points to is not removed. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:146: FrameKey ref_key(frame.picture_id, frame.spatial_layer); On 2016/05/17 16:21:38, danilchap wrote: > frame.spatial_layer - 1 Fixed, also found a bug in the unittest. https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:62: // Keep track of which decoded franes. On 2016/05/17 16:21:39, danilchap wrote: > Keep track of decoded frames? Done.
https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); On 2016/05/18 09:00:56, philipel wrote: > On 2016/05/17 16:21:38, danilchap wrote: > > old_picture_id is oldest allowed picture_id (it's age is exactly MaxAge), so > > (old_picture_id, 0) shouldn't be erased. > > Replacing kMaxSpatialLayers with 0 should fix this issue and allows to remove > > kMaxSpatialLayers constant completely. > > erase removes the elements in the range [begin, old_decoded_it), so the element > old_decoded_it points to is not removed. lower_bound find first element >= (old_picture_id, kMaxSpatialLayers), so if there is an element (old_picture_id, kMaxSpatialLayers - 1) old_decoded_it would point to next element. then erase remove elements in range [begin, old_decoded_it), i.e. in range [begin, old_picture_id] May be add an edge tests case to check this? (i.e. that work when frame is kMaxFrameAge older, but not when it is kMaxFrameAge+1 older: e.g. add two frames where second depends on 1st frame and exactly kMaxFrameAge older, test that you can get out both. That should be faster than creating kMaxFrameAge frames) And/or return old logic where it was more clear how you filter by picture_id. Because kMaxNumHistoryFrames looks stricter than kMaxFrameAge, most of the time loop will be executed 0 times, so erase(begin(), lower_bound) rarely give perfomance benefits.
https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); On 2016/05/18 09:57:59, danilchap wrote: > On 2016/05/18 09:00:56, philipel wrote: > > On 2016/05/17 16:21:38, danilchap wrote: > > > old_picture_id is oldest allowed picture_id (it's age is exactly MaxAge), so > > > (old_picture_id, 0) shouldn't be erased. > > > Replacing kMaxSpatialLayers with 0 should fix this issue and allows to > remove > > > kMaxSpatialLayers constant completely. > > > > erase removes the elements in the range [begin, old_decoded_it), so the > element > > old_decoded_it points to is not removed. > > lower_bound find first element >= (old_picture_id, kMaxSpatialLayers), > so if there is an element (old_picture_id, kMaxSpatialLayers - 1) old_decoded_it > would point to next element. > then erase remove elements in range [begin, old_decoded_it), i.e. in range > [begin, old_picture_id] > > May be add an edge tests case to check this? (i.e. that work when frame is > kMaxFrameAge older, but not when it is kMaxFrameAge+1 older: e.g. add two frames > where second depends on 1st frame and exactly kMaxFrameAge older, test that you > can get out both. That should be faster than creating kMaxFrameAge frames) > > And/or return old logic where it was more clear how you filter by picture_id. > Because kMaxNumHistoryFrames looks stricter than kMaxFrameAge, most of the time > loop will be executed 0 times, so erase(begin(), lower_bound) rarely give > perfomance benefits. I don't want to create a unittest for this since this is an implementation detail. You are right that this can be considered to be an off-by-one error, but here it is not important if we remove history that is |kMaxFrameAge| old since all we need to do is to clean up history that is old according to some arbitrary definition of old.
lgtm https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); On 2016/05/18 10:42:56, philipel wrote: > On 2016/05/18 09:57:59, danilchap wrote: > > On 2016/05/18 09:00:56, philipel wrote: > > > On 2016/05/17 16:21:38, danilchap wrote: > > > > old_picture_id is oldest allowed picture_id (it's age is exactly MaxAge), > so > > > > (old_picture_id, 0) shouldn't be erased. > > > > Replacing kMaxSpatialLayers with 0 should fix this issue and allows to > > remove > > > > kMaxSpatialLayers constant completely. > > > > > > erase removes the elements in the range [begin, old_decoded_it), so the > > element > > > old_decoded_it points to is not removed. > > > > lower_bound find first element >= (old_picture_id, kMaxSpatialLayers), > > so if there is an element (old_picture_id, kMaxSpatialLayers - 1) > old_decoded_it > > would point to next element. > > then erase remove elements in range [begin, old_decoded_it), i.e. in range > > [begin, old_picture_id] > > > > May be add an edge tests case to check this? (i.e. that work when frame is > > kMaxFrameAge older, but not when it is kMaxFrameAge+1 older: e.g. add two > frames > > where second depends on 1st frame and exactly kMaxFrameAge older, test that > you > > can get out both. That should be faster than creating kMaxFrameAge frames) > > > > And/or return old logic where it was more clear how you filter by picture_id. > > Because kMaxNumHistoryFrames looks stricter than kMaxFrameAge, most of the > time > > loop will be executed 0 times, so erase(begin(), lower_bound) rarely give > > perfomance benefits. > > I don't want to create a unittest for this since this is an implementation > detail. > > You are right that this can be considered to be an off-by-one error, but here it > is not important if we remove history that is |kMaxFrameAge| old since all we > need to do is to clean up history that is old according to some arbitrary > definition of old. Good point about unittest - forgot MaxAge is a private constant Agree off-by-one error is not important here. But clarity is. removing kMaxSpatialLayers (that is not used anywhere else) would improve clarity a bit and, as a side-effect, fix off-by-one error. https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:75: }; you may want include constructormagic.h and use RTC_DISALLOW_IMPLICIT_CONSTRUCTORS to indicate this class is not to copy https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2_unittest.cc:21: #include "webrtc/base/platform_thread.h" platform < random
Magnus, PTAL https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:122: decoded_frames_.lower_bound(FrameKey(old_picture_id, kMaxSpatialLayers)); On 2016/05/18 11:46:25, danilchap wrote: > On 2016/05/18 10:42:56, philipel wrote: > > On 2016/05/18 09:57:59, danilchap wrote: > > > On 2016/05/18 09:00:56, philipel wrote: > > > > On 2016/05/17 16:21:38, danilchap wrote: > > > > > old_picture_id is oldest allowed picture_id (it's age is exactly > MaxAge), > > so > > > > > (old_picture_id, 0) shouldn't be erased. > > > > > Replacing kMaxSpatialLayers with 0 should fix this issue and allows to > > > remove > > > > > kMaxSpatialLayers constant completely. > > > > > > > > erase removes the elements in the range [begin, old_decoded_it), so the > > > element > > > > old_decoded_it points to is not removed. > > > > > > lower_bound find first element >= (old_picture_id, kMaxSpatialLayers), > > > so if there is an element (old_picture_id, kMaxSpatialLayers - 1) > > old_decoded_it > > > would point to next element. > > > then erase remove elements in range [begin, old_decoded_it), i.e. in range > > > [begin, old_picture_id] > > > > > > May be add an edge tests case to check this? (i.e. that work when frame is > > > kMaxFrameAge older, but not when it is kMaxFrameAge+1 older: e.g. add two > > frames > > > where second depends on 1st frame and exactly kMaxFrameAge older, test that > > you > > > can get out both. That should be faster than creating kMaxFrameAge frames) > > > > > > And/or return old logic where it was more clear how you filter by > picture_id. > > > Because kMaxNumHistoryFrames looks stricter than kMaxFrameAge, most of the > > time > > > loop will be executed 0 times, so erase(begin(), lower_bound) rarely give > > > perfomance benefits. > > > > I don't want to create a unittest for this since this is an implementation > > detail. > > > > You are right that this can be considered to be an off-by-one error, but here > it > > is not important if we remove history that is |kMaxFrameAge| old since all we > > need to do is to clean up history that is old according to some arbitrary > > definition of old. > > Good point about unittest - forgot MaxAge is a private constant > > Agree off-by-one error is not important here. But clarity is. > removing kMaxSpatialLayers (that is not used anywhere else) would improve > clarity a bit and, as a side-effect, fix off-by-one error. Good point about clarity (and the off by one), removed kMaxSpatialLayers. https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.h:75: }; On 2016/05/18 11:46:25, danilchap wrote: > you may want include constructormagic.h and use > RTC_DISALLOW_IMPLICIT_CONSTRUCTORS to indicate this class is not to copy Done. https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/1969403007/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2_unittest.cc:21: #include "webrtc/base/platform_thread.h" On 2016/05/18 11:46:25, danilchap wrote: > platform < random Done.
LGTM, but do you want Stefan's view of this as well? https://codereview.webrtc.org/1969403007/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/1969403007/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/frame_buffer2.cc:134: for (size_t r = 0; r < frame.num_references; ++r) { Here and below, can you add a brief explanation of why you return false. Just ot make it easy to understand in the future.
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, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1969403007/#ps160001 (title: "Added comment and rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969403007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969403007/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 ========== FrameBuffer for the new jitter buffer. BUG=webrtc:5514 ========== to ========== FrameBuffer for the new jitter buffer. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/a376e70cf9d0df3c35d53533b... ==========
Message was sent while issue was closed.
Description was changed from ========== FrameBuffer for the new jitter buffer. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/a376e70cf9d0df3c35d53533b... ========== to ========== FrameBuffer for the new jitter buffer. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org Committed: https://crrev.com/a376e70cf9d0df3c35d53533b454da542661775b Cr-Commit-Position: refs/heads/master@{#12798} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as a376e70cf9d0df3c35d53533b454da542661775b (presubmit successful).
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a376e70cf9d0df3c35d53533b454da542661775b Cr-Commit-Position: refs/heads/master@{#12798}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.webrtc.org/1991513004/ by honghaiz@webrtc.org. The reason for reverting is: Two tests added by this CL failed in Win DrMemory Full: TestFrameBuffer2.OneLayerStreamReordered - TestFrameBuffer2.WaitForFrame See the link here: https://build.chromium.org/p/client.webrtc/waterfall?builder=Win%20DrMemory%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
