Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124943002:
Added various timestamps to FrameObject. (Closed)
|
Created:
4 years, 5 months ago by philipel Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded various timestamps to FrameObject.
Added various timestamps to the FrameObject class which are needed to calculate
the jitter delay.
BUG=webrtc:5514
Committed: https://crrev.com/b4d31085b4534988ee37a4f2ed680a9ed1b34218
Cr-Commit-Position: refs/heads/master@{#13434}
Patch Set 1 #Patch Set 2 : std::unique_ptr<Clock> instead of Clock*. #
Total comments: 17
Patch Set 3 : Feedback fixes. #Patch Set 4 : Rename ...Timestamp to ...Time. #
Total comments: 2
Patch Set 5 : Renamed |received_timestamp| to |received_time|. #
Messages
Total messages: 26 (7 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
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: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15972)
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_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) If I understand the intended usage correctly, I would prefer calling this variable frame_completed_timestamp. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:28: virtual uint32_t Timestamp() const = 0; Which timestamp is this? From the RTP header? Please add a comment. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:56: int64_t frame_completed_timestamp); Different name in the implementation. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:171: max_nack_count, clock_->TimeInMilliseconds())); This is the received_timestamp, right? Do you want to read the clock here, or should we get the timestamp from the socket for the last packet in the frame? Or maybe it doesn't matter?
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/07 16:12:30, terelius wrote: > If I understand the intended usage correctly, I would prefer calling this > variable frame_completed_timestamp. This timestamp indicates when the last packet of the frame was received, so I think |received_timestamp| is correct. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:28: virtual uint32_t Timestamp() const = 0; On 2016/07/07 16:12:30, terelius wrote: > Which timestamp is this? From the RTP header? Please add a comment. Done. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:56: int64_t frame_completed_timestamp); On 2016/07/07 16:12:30, terelius wrote: > Different name in the implementation. Fixed. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:171: max_nack_count, clock_->TimeInMilliseconds())); On 2016/07/07 16:12:31, terelius wrote: > This is the received_timestamp, right? Do you want to read the clock here, or > should we get the timestamp from the socket for the last packet in the frame? Or > maybe it doesn't matter? I think it might matter actually, gonna look deeper into it.
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/08 09:41:19, philipel wrote: > On 2016/07/07 16:12:30, terelius wrote: > > If I understand the intended usage correctly, I would prefer calling this > > variable frame_completed_timestamp. > > This timestamp indicates when the last packet of the frame was received, so I > think |received_timestamp| is correct. Should it then be called last_packet_time or something? Looking at the code where this object is constructed, it does look more like it's the time when the frame was complete, as it may not necessarily be the last packet if there is reordering. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:30: virtual int64_t RenderTimestamp() const = 0; I'd prefer if we don't overuse the word timestamp as it is tightly connected to the RTP timestamp. Can we instead use RenderTime and ReceivedTime()? Maybe ReceivedTime should be called CompleteTime instead? https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:75: webrtc::Clock* const clock_; You are already in the webrtc namespace.
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > On 2016/07/08 09:41:19, philipel wrote: > > On 2016/07/07 16:12:30, terelius wrote: > > > If I understand the intended usage correctly, I would prefer calling this > > > variable frame_completed_timestamp. > > > > This timestamp indicates when the last packet of the frame was received, so I > > think |received_timestamp| is correct. > > Should it then be called last_packet_time or something? Looking at the code > where this object is constructed, it does look more like it's the time when the > frame was complete, as it may not necessarily be the last packet if there is > reordering. Hmmm... naming here is kind of hard. I have chosen to not call a frame complete until all its packets has been received AND all its references has been found, so a frame that only has all its packets but not its references found is not a complete frame. Maybe it is better to change the convention so that a frame which has all its packets is called complete, and after finding its references something else... But just to be clear, I want to store when the last packet of this frame was received regardless of which order they were received. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:30: virtual int64_t RenderTimestamp() const = 0; On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > I'd prefer if we don't overuse the word timestamp as it is tightly connected to > the RTP timestamp. Can we instead use RenderTime and ReceivedTime()? Maybe > ReceivedTime should be called CompleteTime instead? Done. https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:75: webrtc::Clock* const clock_; On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > You are already in the webrtc namespace. Oops
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/08 11:10:50, philipel wrote: > On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > > On 2016/07/08 09:41:19, philipel wrote: > > > On 2016/07/07 16:12:30, terelius wrote: > > > > If I understand the intended usage correctly, I would prefer calling this > > > > variable frame_completed_timestamp. > > > > > > This timestamp indicates when the last packet of the frame was received, so > I > > > think |received_timestamp| is correct. > > > > Should it then be called last_packet_time or something? Looking at the code > > where this object is constructed, it does look more like it's the time when > the > > frame was complete, as it may not necessarily be the last packet if there is > > reordering. > > Hmmm... naming here is kind of hard. I have chosen to not call a frame complete > until all its packets has been received AND all its references has been found, > so a frame that only has all its packets but not its references found is not a > complete frame. > > Maybe it is better to change the convention so that a frame which has all its > packets is called complete, and after finding its references something else... > > But just to be clear, I want to store when the last packet of this frame was > received regardless of which order they were received. Yes, it's a good idea to try to be clear. In the old JB we use "continuous" to say that a frame has all its references.
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/08 11:34:23, stefan-webrtc (holmer) wrote: > On 2016/07/08 11:10:50, philipel wrote: > > On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > > > On 2016/07/08 09:41:19, philipel wrote: > > > > On 2016/07/07 16:12:30, terelius wrote: > > > > > If I understand the intended usage correctly, I would prefer calling > this > > > > > variable frame_completed_timestamp. > > > > > > > > This timestamp indicates when the last packet of the frame was received, > so > > I > > > > think |received_timestamp| is correct. > > > > > > Should it then be called last_packet_time or something? Looking at the code > > > where this object is constructed, it does look more like it's the time when > > the > > > frame was complete, as it may not necessarily be the last packet if there is > > > reordering. > > > > Hmmm... naming here is kind of hard. I have chosen to not call a frame > complete > > until all its packets has been received AND all its references has been found, > > so a frame that only has all its packets but not its references found is not a > > complete frame. > > > > Maybe it is better to change the convention so that a frame which has all its > > packets is called complete, and after finding its references something else... > > > > But just to be clear, I want to store when the last packet of this frame was > > received regardless of which order they were received. > > Yes, it's a good idea to try to be clear. In the old JB we use "continuous" to > say that a frame has all its references. BTW, might be a good idea to add a comment about the chosen convention in the header file.
https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) On 2016/07/08 11:34:23, stefan-webrtc (holmer) wrote: > On 2016/07/08 11:10:50, philipel wrote: > > On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > > > On 2016/07/08 09:41:19, philipel wrote: > > > > On 2016/07/07 16:12:30, terelius wrote: > > > > > If I understand the intended usage correctly, I would prefer calling > this > > > > > variable frame_completed_timestamp. > > > > > > > > This timestamp indicates when the last packet of the frame was received, > so > > I > > > > think |received_timestamp| is correct. > > > > > > Should it then be called last_packet_time or something? Looking at the code > > > where this object is constructed, it does look more like it's the time when > > the > > > frame was complete, as it may not necessarily be the last packet if there is > > > reordering. > > > > Hmmm... naming here is kind of hard. I have chosen to not call a frame > complete > > until all its packets has been received AND all its references has been found, > > so a frame that only has all its packets but not its references found is not a > > complete frame. > > > > Maybe it is better to change the convention so that a frame which has all its > > packets is called complete, and after finding its references something else... > > > > But just to be clear, I want to store when the last packet of this frame was > > received regardless of which order they were received. > > Yes, it's a good idea to try to be clear. In the old JB we use "continuous" to > say that a frame has all its references. Maybe this convention would work: "received frame" A frame which has received all its packets "complete frame" A frame which has received all its packets and all its references are known. "continuous frame" A frame which have all its packets, all its references known and all its referenced frames decoded.
https://codereview.webrtc.org/2124943002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:84: int64_t received_timestamp_; Since you changed ReceivedTimestamp to ReceivedTime, I think you should do the same for the variable name, i.e. received_time_.
m https://codereview.webrtc.org/2124943002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/2124943002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:84: int64_t received_timestamp_; On 2016/07/11 08:44:08, terelius wrote: > Since you changed ReceivedTimestamp to ReceivedTime, I think you should do the > same for the variable name, i.e. received_time_. Done.
Code LGTM. Waiting for Stefan's opinion on the naming convention w.r.t. received/completed/continuous/decodable frames.
On 2016/07/08 13:40:02, philipel wrote: > https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/frame_object.cc (right): > > https://codereview.webrtc.org/2124943002/diff/20001/webrtc/modules/video_codi... > webrtc/modules/video_coding/frame_object.cc:30: int64_t received_timestamp) > On 2016/07/08 11:34:23, stefan-webrtc (holmer) wrote: > > On 2016/07/08 11:10:50, philipel wrote: > > > On 2016/07/08 10:33:39, stefan-webrtc (holmer) wrote: > > > > On 2016/07/08 09:41:19, philipel wrote: > > > > > On 2016/07/07 16:12:30, terelius wrote: > > > > > > If I understand the intended usage correctly, I would prefer calling > > this > > > > > > variable frame_completed_timestamp. > > > > > > > > > > This timestamp indicates when the last packet of the frame was received, > > so > > > I > > > > > think |received_timestamp| is correct. > > > > > > > > Should it then be called last_packet_time or something? Looking at the > code > > > > where this object is constructed, it does look more like it's the time > when > > > the > > > > frame was complete, as it may not necessarily be the last packet if there > is > > > > reordering. > > > > > > Hmmm... naming here is kind of hard. I have chosen to not call a frame > > complete > > > until all its packets has been received AND all its references has been > found, > > > so a frame that only has all its packets but not its references found is not > a > > > complete frame. > > > > > > Maybe it is better to change the convention so that a frame which has all > its > > > packets is called complete, and after finding its references something > else... > > > > > > But just to be clear, I want to store when the last packet of this frame was > > > received regardless of which order they were received. > > > > Yes, it's a good idea to try to be clear. In the old JB we use "continuous" to > > say that a frame has all its references. > > Maybe this convention would work: > > "received frame" > A frame which has received all its packets > > "complete frame" > A frame which has received all its packets and all its references are known. > > "continuous frame" > A frame which have all its packets, all its references known and all its > referenced frames decoded. sgtm lgtm
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/...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Added various timestamps to FrameObject. Added various timestamps to the FrameObject class which are needed to calculate the jitter delay. BUG=webrtc:5514 ========== to ========== Added various timestamps to FrameObject. Added various timestamps to the FrameObject class which are needed to calculate the jitter delay. BUG=webrtc:5514 Committed: https://crrev.com/b4d31085b4534988ee37a4f2ed680a9ed1b34218 Cr-Commit-Position: refs/heads/master@{#13434} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b4d31085b4534988ee37a4f2ed680a9ed1b34218 Cr-Commit-Position: refs/heads/master@{#13434} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
