Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
56333003:
Adding transport to Cast/test (Closed)
|
Created:
7 years, 1 month ago by mikhal Modified:
7 years, 1 month ago Reviewers:
Alpha Left Google CC:
chromium-reviews, feature-media-reviews_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding transport to Cast/test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233220
Patch Set 1 #Patch Set 2 : Fix loss calculation #
Total comments: 38
Patch Set 3 : Responding to review #
Total comments: 6
Patch Set 4 : Responding to review #
Messages
Total messages: 16 (0 generated)
Please review, -Mikhal
Looks pretty good. Just have a few minor comments. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... File media/cast/test/transport/transport.cc (right): https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:7: #include <stdio.h> What is this for? I don't see anything related to this header. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:8: #include <stdlib.h> #include <string> instead, it will give you memcpy. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:10: #include <cassert> Don't use <cassert>, use base/logging.h instead which give you CHECK(). https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:16: #include "base/message_loop/message_loop.h" I don't see this being used. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:29: typedef std::vector<unsigned char> IPAddressNumber; This is already defined as net::IPAddressNumber. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:33: IPAddressNumber ip_number; Use net::IPAddressNumber instead. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:48: delete [] packet; If you pass in buffer_data->data() to packet_receiver_ at line 60 then you don't need to delete this packet. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:60: memcpy(data, reinterpret_cast<uint8*>(buffer_->data()), result); Don't do this memcpy just pass buffer_->data() to packet_receiver_. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:66: void RecvFromSocketLoop() { This is good! Now you know how this async read loop works. :) https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:71: if (res > 0 ) { nit: (res > 0) https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:93: nit: remove this empty line. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:101: bool UniformLoss(double loss_pct) { This is not being used. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:106: nit: remove this extra space. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:110: int r = base::RandInt(0, 100); Maybe use UniformLoss() instead? https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:121: int rv = udp_socket_->SendTo( It will be more clear if you do this: #include "base/bind_helpers.h" and then base::Bind(&DoNothing); DoNothing() is defined in bind_helpers.h that really do nothing. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:127: bool out_val = true; nit: There's two space after _val. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:136: assert(percentage >= 0); Use CHECK() instead of assert(). https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:160: static_cast<LocalPacketSender*>(sender)->SetPacketLoss(percentage); Since packet_sender_ is always LocalPacketSender I suggest you make packet_sender_ be scoped_ptr<LocalPacketSender>. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... File media/cast/test/transport/transport.h (right): https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.h:14: nit: remove this extra space. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.h:27: // Specifies the ports and IP address to receive packets on. nit: s/ports/port/. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.h:36: PacketSender* packet_sender() {return packet_sender_.get();} nit: { return packet_sender_.get(); } Give a space between {}. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.h:44: scoped_ptr<PacketSender> packet_sender_; Why not just use scoped_ptr<LocalPacketSender>? You can forward declare it at the top.
PTAL https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... File media/cast/test/transport/transport.cc (right): https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:7: #include <stdio.h> On 2013/11/01 23:23:19, Alpha wrote: > What is this for? I don't see anything related to this header. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:8: #include <stdlib.h> On 2013/11/01 23:23:19, Alpha wrote: > #include <string> instead, it will give you memcpy. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:10: #include <cassert> On 2013/11/01 23:23:19, Alpha wrote: > Don't use <cassert>, use base/logging.h instead which give you CHECK(). Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:16: #include "base/message_loop/message_loop.h" On 2013/11/01 23:23:19, Alpha wrote: > I don't see this being used. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:29: typedef std::vector<unsigned char> IPAddressNumber; On 2013/11/01 23:23:19, Alpha wrote: > This is already defined as net::IPAddressNumber. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:33: IPAddressNumber ip_number; On 2013/11/01 23:23:19, Alpha wrote: > Use net::IPAddressNumber instead. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:60: memcpy(data, reinterpret_cast<uint8*>(buffer_->data()), result); On 2013/11/01 23:23:19, Alpha wrote: > Don't do this memcpy just pass buffer_->data() to packet_receiver_. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:66: void RecvFromSocketLoop() { thanks! On 2013/11/01 23:23:19, Alpha wrote: > This is good! Now you know how this async read loop works. :) https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:71: if (res > 0 ) { On 2013/11/01 23:23:19, Alpha wrote: > nit: (res > 0) Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:93: On 2013/11/01 23:23:19, Alpha wrote: > nit: remove this empty line. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:101: bool UniformLoss(double loss_pct) { removed On 2013/11/01 23:23:19, Alpha wrote: > This is not being used. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:106: On 2013/11/01 23:23:19, Alpha wrote: > nit: remove this extra space. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:110: int r = base::RandInt(0, 100); Removed uniform loss as we only have one type of loss. On 2013/11/01 23:23:19, Alpha wrote: > Maybe use UniformLoss() instead? https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:127: bool out_val = true; On 2013/11/01 23:23:19, Alpha wrote: > nit: There's two space after _val. Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:136: assert(percentage >= 0); On 2013/11/01 23:23:19, Alpha wrote: > Use CHECK() instead of assert(). Done. https://chromiumcodereview.appspot.com/56333003/diff/40001/media/cast/test/tr... media/cast/test/transport/transport.cc:160: static_cast<LocalPacketSender*>(sender)->SetPacketLoss(percentage); On 2013/11/01 23:23:19, Alpha wrote: > Since packet_sender_ is always LocalPacketSender I suggest you make > packet_sender_ be scoped_ptr<LocalPacketSender>. Done.
https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... File media/cast/test/transport/transport.cc (right): https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:42: void DeletePacket(const uint8* packet) { Remove this method as it is not needed. https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:55: base::Bind(&LocalUdpTransportData::DeletePacket, That's not right. You don't need to delete the packet here as it is owned by buffer_. Just bind to RecvFromSocketLoop directly. https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:64: if (res > 0) { What happens if there's an error? The read loop will be interrupted.
ptal https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... File media/cast/test/transport/transport.cc (right): https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:42: void DeletePacket(const uint8* packet) { True.Done. On 2013/11/04 19:51:09, Alpha wrote: > Remove this method as it is not needed. https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:55: base::Bind(&LocalUdpTransportData::DeletePacket, On 2013/11/04 19:51:09, Alpha wrote: > That's not right. You don't need to delete the packet here as it is owned by > buffer_. Just bind to RecvFromSocketLoop directly. Done. https://codereview.chromium.org/56333003/diff/190001/media/cast/test/transpor... media/cast/test/transport/transport.cc:64: if (res > 0) { On 2013/11/04 19:51:09, Alpha wrote: > What happens if there's an error? The read loop will be interrupted. Done.
LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@google.com/56333003/190002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@google.com/56333003/190002
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@google.com/56333003/190002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@google.com/56333003/190002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@google.com/56333003/190002
Message was sent while issue was closed.
Change committed as 233220 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
