Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199133004:
PacketBuffer is now ref counted. (Closed)
|
Created:
4 years, 4 months ago by philipel Modified:
4 years, 4 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. |
DescriptionPacketBuffer is now ref counted.
Since all FrameObjects have a reference to its PacketBuffer and since
the PacketBuffer can be thrown away at any moment the PacketBuffer
has to be ref counted in order to avoid FrameObjects dereferencing a potentially
destroyed object.
BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/17deeb47edf9d63cdb827c2295762c956bbe31e5
Patch Set 1 #
Total comments: 4
Patch Set 2 : PacketBuffer/RtpFrameReferenceFinder can now be stopped. #
Total comments: 2
Patch Set 3 : Moved RtpFrameReferenceFinder out of PacketBuffer. #
Total comments: 28
Patch Set 4 : PacketBufferFake -> FakePacketBuffer + removed old comments. #
Total comments: 21
Patch Set 5 : Feedback fixes. #
Total comments: 2
Patch Set 6 : Nit fix #
Total comments: 2
Patch Set 7 : Nit fixes + rebase #Patch Set 8 : #include <utility> for std::move #
Messages
Total messages: 52 (24 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, holmer@chromium.org
https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:181: reference_finder_.ManageFrame(std::move(frame)); this look like a circular dependency: { auto buffer = PacketBuffer::Create(); // 1 reference. buffer->findFrames(...); // create a frame, store it in reference_finder_ stashed_frames_ queue, now PacketBuffer has 2 references. } // destroy buffer scoped_refptr, reducing PacketBuffer reference counter to 1. // Now there is an unreachable PacketBuffer that contain refernce_finder_ with 1 RtpFrameObject that holds that single reference to PacketBuffer. https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:40: class PacketBuffer : rtc::RefCountInterface { Do you need to derive from this interface? If you plan to use PacketBuffer with scoped_refptr only, then you do not. But if you have some other plan, make it a public derivation.
Hmm.. to fix the circular dependency problem I added a Stop function to PacketBuffer/RtpFrameReferenceFinder which clears all frames. But now that I think of it, I wonder if a better design would be to move the RtpFrameReferenceFinder out of the PacketBuffer so that it can be destroyed at an appropriate time. WDYT danilchap@? https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:181: reference_finder_.ManageFrame(std::move(frame)); On 2016/08/02 14:04:45, danilchap wrote: > this look like a circular dependency: > { > auto buffer = PacketBuffer::Create(); // 1 reference. > buffer->findFrames(...); // create a frame, store it in reference_finder_ > stashed_frames_ queue, now PacketBuffer has 2 references. > } // destroy buffer scoped_refptr, reducing PacketBuffer reference counter to 1. > // Now there is an unreachable PacketBuffer that contain refernce_finder_ with 1 > RtpFrameObject that holds that single reference to PacketBuffer. You are right, the risk of having a stashed frame is significant. I have added a Stop function to PacketBuffer/RtpFrameReferenceFinder which clears all stashed frames. https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:40: class PacketBuffer : rtc::RefCountInterface { On 2016/08/02 14:04:45, danilchap wrote: > Do you need to derive from this interface? > If you plan to use PacketBuffer with scoped_refptr only, then you do not. > But if you have some other plan, make it a public derivation. Removed inheritance.
Thinking about similar solution: split PacketBuffer into two parts. One is 'static' (and keep RtpFrameRefernceFinder) - destroyed when external user tells PacketBuffer is no longer needed (instead of calling Stop function) Another one is ref-counted - it contain only the data that individual RtpFrameObjects needs to access not to crash. The ownership dependency might be more straightforward then: PacketBuffer owns RtpFrameReferenceFinder and so some RtpFrameObject objects PacketBuffer and RtpFrameObjects share ownership of PacketBufferData. Does that structure make sense? btw, I assume that PacketBuffer can't control lifetime of frame objects it creates i.e. some other module able to take ownership of an RtpFrameObject. https://codereview.webrtc.org/2199133004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:17: #include "webrtc/base/refcount.h" not needed now https://codereview.webrtc.org/2199133004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2199133004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:66: while (!stashed_frames_.empty()) stashed_frames_.clear() instead? or order of destruction is so important you want to stress it and protect against future changes? May be need to add a comment then.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm, but please get an lg from danil too. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:27: class PacketBufferFake : public PacketBuffer { FakePacketBuffer https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:1127: // sn , false, frst, lst, intr, pid , sid, tid, tl0, refs Something should probably be done with these comments now that they're no longer aligned
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/...
https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:27: class PacketBufferFake : public PacketBuffer { On 2016/08/04 14:32:30, stefan-webrtc (holmer) wrote: > FakePacketBuffer Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:1127: // sn , false, frst, lst, intr, pid , sid, tid, tl0, refs On 2016/08/04 14:32:30, stefan-webrtc (holmer) wrote: > Something should probably be done with these comments now that they're no longer > aligned Removed comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7192)
not a full review yet, just some nits https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:72: // If this is a padding or FEC packet, don't insert it. Do you want to insert padding and FEC packets now? or you expect (DCHECK then) those will never be inserted. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:48: virtual bool InsertPacket(const VCMPacket& packet); add a comment that you make it virtual for testing. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); destructor should be before other functions (including static one) https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:133: OnReceivedFrameCallback* received_frame_callback_ GUARDED_BY(crit_); OnReceivedFrameCallback* const received_frame_callback_; the variable is never changed, so you do not need to GUARD it. may be you mean PT_GUARDED_BY? https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:59: ref_packet_buffer_(new rtc::RefCountedObject<PacketBufferFake>()), PacketBufferFake inherited from PacketBuffer that now has AddRef/Release functions, i.e. no need to wrap it into RefCountedObject https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:35: uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint16_t>::max()); } may be not topic for this CL, but this line can look neater: uint16_t Rand() { return rand_.Rand<uint16_t>(); } may be that would allow to remove <limits> include. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:70: if (frame_it == frames_from_callback_.end()) { look like you mean here ASSERT_TRUE(frame_it != frames_from_callback_.end()) << "Could not find frame ..."; https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:73: return; at least this one is not needed any more https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:106: uint16_t sn = Rand(); https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Lots of things can be abbreviated "sn" use old name, or may be even full name (doesn't look like full name would cause any line becoming too long): uint16_t sequence_number = Rand();
More proper review, concentrating on the CL's topic (looks good) and changes (see comments) instead of nits. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:1127: // sn , false, frst, lst, intr, pid , sid, tid, tl0, refs On 2016/08/04 14:45:02, philipel wrote: > On 2016/08/04 14:32:30, stefan-webrtc (holmer) wrote: > > Something should probably be done with these comments now that they're no > longer > > aligned > > Removed comments. Now it it hard to understand what frames do you insert. May be align parameters and comments instead like it was before. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:72: // If this is a padding or FEC packet, don't insert it. May be add function OnReceivedFrameCallback::OnReceivedPadding(seq_num) to keep this CL concentrated on introducing ref counting and leaving other functionality unchanged. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:14: #include <vector> add #include <memory> since you are using std::unique_ptr https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); Move destructor to the top https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:27: class FakePacketBuffer : public PacketBuffer { might be better to put FakePacketBuffer into unnamed namespace. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:55: public OnCompleteFrameCallback { it might be cleaner to split callback object from the test class: class FrameCallback : public OnCompleteFrameCallback { public: void OnCompleteFrame(std::unique_ptr<FrameObject> frame) override { ... } size_t FramesNumber() const { return frames_.size(); bool HasFrame(pid, sidx) const { ... } private: std::map<> frames_; }; } // namespace class TestRtpFrameReferenceFinder : public ::testing::Test { ... FramesCallback callback_; }; ... ASSERT_TRUE(callback_.HasFrame(pid, sidx)); ... EXPECT_EQ(2u, callback_.FramesNumber()); https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:50: void InsertPacket(uint16_t seq_num, // packet sequence number without additional comment in each test case it is now hard to understand what kind of packets are inserted (what does that true/false combinations mean?) Alternative to comments way is to accept enums with descriptive names instead of raw bool values: https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:51: bool keyframe, // is keyframe enum IsKeyFrame { kKeyFrame, kDeltaFrame } or accepts already existent enum with values kVideFrameKey/kVideFrameDelta. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:52: bool first, // is first packet of frame enum IsFirst { kFirst, kNotFirst } https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:53: bool last, // is last packet of frame enum IsLast { kLast, kNotLast } https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:54: int data_size = 0, // size of data as for passing buffer I can recommend to check ArrayView class to accept as a parameter: rtc::ArrayView<const uint8_t> data = nullptr (instead of two parameters). https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:139: uint16_t sn = Rand(); I find seq_num and alligned comment for explaining parameters more clear than changed version. (true/false looks better than kT/kF) May be do not do any other changes other than replacing InsertGeneric with InsertPacket call?
https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:72: // If this is a padding or FEC packet, don't insert it. On 2016/08/04 16:03:09, danilchap wrote: > Do you want to insert padding and FEC packets now? > or you expect (DCHECK then) those will never be inserted. This was only used to forwards information to the RtpFrameReferenceFinder, but still it might be good to guard against someone trying to insert a FEC or padding packet. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:48: virtual bool InsertPacket(const VCMPacket& packet); On 2016/08/04 16:03:09, danilchap wrote: > add a comment that you make it virtual for testing. Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); On 2016/08/04 16:03:09, danilchap wrote: > destructor should be before other functions (including static one) Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:133: OnReceivedFrameCallback* received_frame_callback_ GUARDED_BY(crit_); On 2016/08/04 16:03:09, danilchap wrote: > OnReceivedFrameCallback* const received_frame_callback_; > the variable is never changed, so you do not need to GUARD it. > may be you mean PT_GUARDED_BY? Now const and removed GUARDED_BY https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:59: ref_packet_buffer_(new rtc::RefCountedObject<PacketBufferFake>()), On 2016/08/04 16:03:09, danilchap wrote: > PacketBufferFake inherited from PacketBuffer that now has AddRef/Release > functions, i.e. no need to wrap it into RefCountedObject Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:1127: // sn , false, frst, lst, intr, pid , sid, tid, tl0, refs On 2016/08/05 12:50:50, danilchap wrote: > On 2016/08/04 14:45:02, philipel wrote: > > On 2016/08/04 14:32:30, stefan-webrtc (holmer) wrote: > > > Something should probably be done with these comments now that they're no > > longer > > > aligned > > > > Removed comments. > > Now it it hard to understand what frames do you insert. > May be align parameters and comments instead like it was before. I can add comments about the order of the parameters, but I don't think it's worth aligning them since I can't use git cl format. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:35: uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint16_t>::max()); } On 2016/08/04 16:03:10, danilchap wrote: > may be not topic for this CL, but this line can look neater: > uint16_t Rand() { return rand_.Rand<uint16_t>(); } > may be that would allow to remove <limits> include. Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:70: if (frame_it == frames_from_callback_.end()) { On 2016/08/04 16:03:09, danilchap wrote: > look like you mean here > ASSERT_TRUE(frame_it != frames_from_callback_.end()) > << "Could not find frame ..."; Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:73: return; On 2016/08/04 16:03:10, danilchap wrote: > at least this one is not needed any more Done. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:106: uint16_t sn = Rand(); On 2016/08/04 16:03:09, danilchap wrote: > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > Lots of things can be abbreviated "sn" > use old name, or may be even full name (doesn't look like full name would cause > any line becoming too long): > uint16_t sequence_number = Rand(); Reverted back to seq_num. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:72: // If this is a padding or FEC packet, don't insert it. On 2016/08/05 12:50:50, danilchap wrote: > May be add function > OnReceivedFrameCallback::OnReceivedPadding(seq_num) > to keep this CL concentrated on introducing ref counting and leaving other > functionality unchanged. This functionality is not used anywhere yet so changing it doesn't really matter. When implementing the experiment I just have to notify the RtpFrameRefenceFinder of the recived padding/fec packets and not insert them into the PacketBuffer. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); On 2016/08/05 12:50:50, danilchap wrote: > Move destructor to the top Done. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:27: class FakePacketBuffer : public PacketBuffer { On 2016/08/05 12:50:50, danilchap wrote: > might be better to put FakePacketBuffer into unnamed namespace. Acknowledged. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:55: public OnCompleteFrameCallback { On 2016/08/05 12:50:50, danilchap wrote: > it might be cleaner to split callback object from the test class: > > class FrameCallback : public OnCompleteFrameCallback { > public: > void OnCompleteFrame(std::unique_ptr<FrameObject> frame) override { ... } > size_t FramesNumber() const { return frames_.size(); > bool HasFrame(pid, sidx) const { ... } > private: > std::map<> frames_; > }; > } // namespace > > class TestRtpFrameReferenceFinder : public ::testing::Test { > ... > FramesCallback callback_; > }; > > ... > ASSERT_TRUE(callback_.HasFrame(pid, sidx)); > ... > EXPECT_EQ(2u, callback_.FramesNumber()); I don't think it makes much of a difference in terms of code complexity/readability. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:50: void InsertPacket(uint16_t seq_num, // packet sequence number On 2016/08/05 12:50:51, danilchap wrote: > without additional comment in each test case it is now hard to understand what > kind of packets are inserted (what does that true/false combinations mean?) > Alternative to comments way is to accept enums with descriptive names instead of > raw bool values: Done. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:51: bool keyframe, // is keyframe On 2016/08/05 12:50:51, danilchap wrote: > enum IsKeyFrame { kKeyFrame, kDeltaFrame } > or accepts already existent enum with values kVideFrameKey/kVideFrameDelta. Done. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:52: bool first, // is first packet of frame On 2016/08/05 12:50:51, danilchap wrote: > enum IsFirst { kFirst, kNotFirst } Done. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:53: bool last, // is last packet of frame On 2016/08/05 12:50:51, danilchap wrote: > enum IsLast { kLast, kNotLast } Done. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:54: int data_size = 0, // size of data On 2016/08/05 12:50:51, danilchap wrote: > as for passing buffer I can recommend to check ArrayView class to accept as a > parameter: > rtc::ArrayView<const uint8_t> data = nullptr (instead of two parameters). Acknowledged. https://codereview.webrtc.org/2199133004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:139: uint16_t sn = Rand(); On 2016/08/05 12:50:50, danilchap wrote: > I find seq_num and alligned comment for explaining parameters more clear than > changed version. > (true/false looks better than kT/kF) > May be do not do any other changes other than replacing InsertGeneric with > InsertPacket call? Acknowledged.
The change lgtm % nits https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:16: #include "webrtc/base/checks.h" #include "webrtc/base/atomicops.h" https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); On 2016/08/08 13:34:57, philipel wrote: > On 2016/08/04 16:03:09, danilchap wrote: > > destructor should be before other functions (including static one) > > Done. Almost, unless you treat Create as a constructor. https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:1127: // sn , false, frst, lst, intr, pid , sid, tid, tl0, refs On 2016/08/08 13:34:57, philipel wrote: > On 2016/08/05 12:50:50, danilchap wrote: > > On 2016/08/04 14:45:02, philipel wrote: > > > On 2016/08/04 14:32:30, stefan-webrtc (holmer) wrote: > > > > Something should probably be done with these comments now that they're no > > > longer > > > > aligned > > > > > > Removed comments. > > > > Now it it hard to understand what frames do you insert. > > May be align parameters and comments instead like it was before. > > I can add comments about the order of the parameters, but I don't think it's > worth aligning them since I can't use git cl format. fyi: // clang-format off Customly formatted text aligned the way you want. // clang-format on git cl format skips those blocks. presubmit still validates them. https://codereview.webrtc.org/2199133004/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:51: // Virtual for testing .
Good tip about //clang-format off/on :) https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:55: virtual ~PacketBuffer(); On 2016/08/08 14:49:31, danilchap wrote: > On 2016/08/08 13:34:57, philipel wrote: > > On 2016/08/04 16:03:09, danilchap wrote: > > > destructor should be before other functions (including static one) > > > > Done. > > Almost, > unless you treat Create as a constructor. I think it's reasonable to consider the Create function to be the constructor of this class. https://codereview.webrtc.org/2199133004/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2199133004/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:51: // Virtual for testing On 2016/08/08 14:49:31, danilchap wrote: > . Done.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2199133004/#ps100001 (title: "Nit fix")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7248)
philipel@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, a lgtm for webrtc/modules/BUILD.gn please :)
One nit / kind of unrelated comment for the gyp file, but LGTM for BUID.gn. https://codereview.webrtc.org/2199133004/diff/100001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/2199133004/diff/100001/webrtc/modules/modules.g... webrtc/modules/modules.gyp:379: 'video_coding/video_packet_buffer_unittest.cc', video_coding/video_packet_buffer_unittest.cc is not in alphabetic order. I know this isn't from this change, but I guess it was added in one of your previous CLs and it would be good to move.
https://codereview.webrtc.org/2199133004/diff/100001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/2199133004/diff/100001/webrtc/modules/modules.g... webrtc/modules/modules.gyp:379: 'video_coding/video_packet_buffer_unittest.cc', On 2016/08/09 12:25:05, mflodman wrote: > video_coding/video_packet_buffer_unittest.cc is not in alphabetic order. I know > this isn't from this change, but I guess it was added in one of your previous > CLs and it would be good to move. Sorted all files in sources section.
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, stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2199133004/#ps120001 (title: "Nit fixes + rebase")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7286)
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, stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2199133004/#ps140001 (title: "#include <utility> for std::move")
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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/13153)
The CQ bit was checked by philipel@webrtc.org
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_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...)
The CQ bit was checked by philipel@webrtc.org
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: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1715) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1737) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15643)
The CQ bit was checked by philipel@webrtc.org
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)
Description was changed from ========== PacketBuffer is now ref counted. Since all FrameObjects have a reference to its PacketBuffer and since the PacketBuffer can be thrown away at any moment the PacketBuffer has to be ref counted in order to avoid FrameObjects dereferencing a potentially destroyed object. BUG=webrtc:5514 ========== to ========== PacketBuffer is now ref counted. Since all FrameObjects have a reference to its PacketBuffer and since the PacketBuffer can be thrown away at any moment the PacketBuffer has to be ref counted in order to avoid FrameObjects dereferencing a potentially destroyed object. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/17deeb47edf9d63cdb827c229... ==========
Message was sent while issue was closed.
Description was changed from ========== PacketBuffer is now ref counted. Since all FrameObjects have a reference to its PacketBuffer and since the PacketBuffer can be thrown away at any moment the PacketBuffer has to be ref counted in order to avoid FrameObjects dereferencing a potentially destroyed object. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/17deeb47edf9d63cdb827c229... ========== to ========== PacketBuffer is now ref counted. Since all FrameObjects have a reference to its PacketBuffer and since the PacketBuffer can be thrown away at any moment the PacketBuffer has to be ref counted in order to avoid FrameObjects dereferencing a potentially destroyed object. BUG=webrtc:5514 R=danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/17deeb47edf9d63cdb827c2295762c956bbe31e5 Cr-Commit-Position: refs/heads/master@{#13725} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 17deeb47edf9d63cdb827c2295762c956bbe31e5 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
