966625 - [RTSP] Data usage of RTSP should be tracked

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

Currently, RTSP doesn't use the NetworkActivityMonitor and its data usage is not tracked.

Steve suggests:
Convert our RTSP code to use NSPR/PRFileDesc types and functions. Then we can just use the NetworkActivityMonitor functions directly. The NSPR types and functions have a very similar API to standard sockets
This approach also helps us move towards integration with nsSocketTransportService (STS), and cross-platform RTSP support.

Comment 1

[Jason Smith [:jsmith]]

This doesn't fall under QC's FC list & DSDS 1.4 list, so moving to backlog.

Comment 2

[howie [:howie]]

Hi Ethan, do you have resource or any plan to solve this? Thanks.

Comment 3

[Ethan Tseng [:ethan]]

(In reply to howie from comment #2)
> Hi Ethan, do you have resource or any plan to solve this? Thanks.
For now, I don't plan to deal with this bug in the near future.
In v1.5 or v2.0, our focus of RTSP will be:
1. Enable RTSP video feature (which means to fix all the remaining issues of playing RTSP video)
2. Establish automation test framework

Both these targets require a lot of works and have higher priority than this bug.
I think we don't have resource to resolve this bug within a quarter. :)

Comment 4

[Ken Chang[:kenkai]]

This issue isn't in 1.5 scope.

Comment 5

[John Shih]

I can take this! Thanks

Comment 6

[John Shih]

Attachment: Bug 966625 - Make RTPS be tracked by NetworkActivityMonitor.

Since Bug 996535 is landed, this work can easily be done.
Note, this is kind of work around before we use nsSocketTransport dircetly in RTSP.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8421620 [details] [diff] [review]
Bug 966625 - Make RTPS be tracked by NetworkActivityMonitor.

Review of attachment 8421620 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r=me.

(minor nit: I'd prefer if you added r=sworkman when the review is complete, not before :) )

::: netwerk/protocol/rtsp/rtsp/ARTSPConnection.cpp
@@ +272,5 @@
>      }
>  
>      mSocket = PR_OpenTCPSocket(PR_AF_INET);
>  
> +    NetworkActivityMonitor::AttachIOLayer(mSocket);

Please check that mSocket is not null after PR_OpenTCPSocket.

Comment 8

[John Shih]

(In reply to Steve Workman [:sworkman] from comment #7)
> Comment on attachment 8421620 [details] [diff] [review]
> Bug 966625 - Make RTPS be tracked by NetworkActivityMonitor.
> 
> Review of attachment 8421620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! r=me.
> 
> (minor nit: I'd prefer if you added r=sworkman when the review is complete,
> not before :) )

Oops! I'll be careful next time!

Anyway, thanks for your quick response again ;)

> 
> ::: netwerk/protocol/rtsp/rtsp/ARTSPConnection.cpp
> @@ +272,5 @@
> >      }
> >  
> >      mSocket = PR_OpenTCPSocket(PR_AF_INET);
> >  
> > +    NetworkActivityMonitor::AttachIOLayer(mSocket);
> 
> Please check that mSocket is not null after PR_OpenTCPSocket.

Comment 9

[John Shih]

try result: https://tbpl.mozilla.org/?tree=Try&rev=8abc1aeab03c
Looks good!

Comment 10

[John Shih]

Attachment: Bug 966625 - Make RTPS be tracked by NetworkActivityMonitor.

r=sworkman

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/38c19f1581e7

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38c19f1581e7

Comment 13

[William Hsu [:whsu]]

Using following scenario to verify it.
Thanks.

* Steps
  1. launch usage app, and enable network traffic measurement
  2. launch the browser and input the following link on URL bar.
     - http://goo.gl/TwhnSh
  3. After the streaming is stopped, checking the data usage of usage app
  4. Replay the RTSP again
  5. Check the data usage again after the streaming is stopped

* Result:
  Step 3. Increase 4 MB data usage
  Step 5. Increase double data usage (4 MB*2)

* Build information:
 - Gaia      c462d9183d294a2d8ecc472f593ea8cfa15bc9de
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/9d8d16695f6a
 - BuildID   20140520160203
 - Version   32.0a1

Comment 14

[Ethan Tseng [:ethan]]

(In reply to William Hsu [:whsu] from comment #13)
> Using following scenario to verify it.
> Thanks.

Thanks for John and William!
Good job! :)