Bug 95193 – Implement the GetStats interface on PeerConnection

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Harald Alvestrand]

Implement an interface to access statistics on the components of a PeerConnection.

Proposal, not yet incorporated into the PeerConnection API:

https://docs.google.com/a/google.com/document/d/1D71z6ylu39FF--F4JWqIIWFNKbQ3isSLd0wQ2ahvT_4/edit

[Harald Alvestrand]

*** Bug 96852 has been marked as a duplicate of this bug. ***

[Harald Alvestrand]

Created attachment 164764 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 164764 [details]
Patch

Attachment 164764 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13952108

[Tommy Widenflycht]

Comment on attachment 164764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164764&action=review

> Source/Platform/ChangeLog:9
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Fix or remove.

> Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:61
> +    virtual void getStats(const WebRTCStatsRequest&)

Make this a one-liner.

> Source/Platform/chromium/public/WebRTCStatsRequest.h:35
> +#include "WebNonCopyable.h"

Unused

> Source/Platform/chromium/public/WebRTCStatsRequest.h:46
> +

no empty line here

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:387
> +    // TODO: Add use of selector, either in the request or as argument.

TODO -> FIXME

I would prefer if the selector ends up in the request when you fix this.

> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:37
> +        // readonly attribute Dictionary stats;

Firstly this is not possible, and secondly we never check in commented out code in webkit.

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:50
> +      m_successCallback(callback)

the comma goes under the colon.

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:61
> +        // TODO: Fill in content of stats parameter.

TODO -> FIXME

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.h:46
> +class RTCStatsRequestImpl : public RTCStatsRequest, public ActiveDOMObject {

You need to override the stop method from ActiveDOMObject and in it clear the callback.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:36
> +    // TODO: Pass and populate from incoming arg.

FIXME

> Source/WebCore/platform/mediastream/RTCPeerConnectionHandler.h:-68
> -

Leave the empty line.

> Source/WebCore/platform/mediastream/RTCStatsRequest.cpp:52
> +void RTCStatsRequest::requestSucceeded()

This is a pure virtual... Remove implementation.

> Source/WebCore/platform/mediastream/RTCStatsRequest.h:42
> +class RTCStatsRequest : public RefCounted<RTCStatsRequest> {

This class doesn't need a cpp file, and definitely shouldn't have a create method. Look at RTCSessionDescriptionRequest.

> Source/WebCore/platform/mediastream/RTCStatsRequest.h:50
> +

Remove empty line.

[Tommy Widenflycht]

To make this compile under gtk you need to add the files to two more build files in Source/WebCore.
Grep for RTCSessionDescriptionRequest for example in that directory.

[Tommy Widenflycht]

Also, you are using a mix of different licenses. Please use this one for all new files:
 
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp

[Adam Barth]

Comment on attachment 164764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164764&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
> +        void getStats(in [Callback, Optional=DefaultIsUndefined] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);

Is the successCallback really optional?  That seems sort of pointless.  :)

> Source/WebCore/Modules/mediastream/RTCStatsElementList.idl:33
> +    interface [
> +        Conditional=MEDIA_STREAM,
> +        IndexedGetter
> +    ] RTCStatsElementList {
> +        readonly attribute unsigned long length;
> +        RTCStatsElement item(in [IsIndex] unsigned long index);
> +    };

Why not just use Sequence<RTCStatsElement> ?  That's generally preferred these days so that we'll use a real JavaScript array rather than one of these fake DOM-objects-that-try-to-act-like-arrays

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:42
> +private:
> +    RTCStatsReport();
> +};

This class has no storage?

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:60
> +    if (m_successCallback) {

Prefer early return

> Source/WebCore/Modules/mediastream/RTCStatsResponse.idl:36
> +        readonly attribute RTCStatsReportList result;

sequence<RTCStatsReport> ?

[Adam Barth]

Comment on attachment 164764 [details]
Patch

I didn't see RTCStatsReport.cpp.  Maybe you forgot to "svn add" the file?

[Harald Alvestrand]

Created attachment 165126 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165126 [details]
Patch

Attachment 165126 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13951692

[Adam Barth]

Comment on attachment 165126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165126&action=review

This basically looks good.  Is there a reason why you're not using sequence<Foo> instead of creating these fake DOM-based arrays?  Also, it looks like the patch doesn't compile, perhaps because it is missing the CPP file noted below.

> Source/Platform/chromium/public/WebRTCStatsRequest.h:60
> +    WEBKIT_EXPORT void requestSucceeded() const;

Don't we need to supply some stats somehow?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:89
> +    void getStats(PassRefPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);

I would have given these parameters names since it's not clear what they mean otherwise.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
> +        void getStats(in [Callback] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);

Do you have a link to the spec handy?

> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:31
> +        // name "value" causes an error

What does this comment mean?

> Source/WebCore/Modules/mediastream/RTCStatsElementList.h:39
> +    // DOM methods & attributes for RTCStatsElementList

This comment isn't needed.

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:32
> +class RTCStatsReport : public RefCounted<RTCStatsReport> {

I still don't see RTCStatsReport.cpp in this patch.  Is that what's causing the compile error?

[Harald Alvestrand]

Created attachment 165315 [details]
Patch

[WebKit Review Bot]

Attachment 165315 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/Modules/mediastream/RTCStatsResponse.h:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 165315 [details]
Patch

Attachment 165315 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13988556

[WebKit Review Bot]

Comment on attachment 165315 [details]
Patch

Attachment 165315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13993555

[Harald Alvestrand]

Comment on attachment 165126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165126&action=review

>> Source/Platform/chromium/public/WebRTCStatsRequest.h:60
>> +    WEBKIT_EXPORT void requestSucceeded() const;
> 
> Don't we need to supply some stats somehow?

Next patch. I've expanded this patch to supply the storage.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:89
>> +    void getStats(PassRefPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);
> 
> I would have given these parameters names since it's not clear what they mean otherwise.

Done.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
>> +        void getStats(in [Callback] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);
> 
> Do you have a link to the spec handy?

Dated link: http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
Should this be in the code somewhere? Perhaps just in the changelog?

>> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:31
>> +        // name "value" causes an error
> 
> What does this comment mean?

Reminder to self: I can't call this function "value", because that causes an obscure error. So I call it "stat". Moved to personal notebook.

>> Source/WebCore/Modules/mediastream/RTCStatsElementList.h:39
>> +    // DOM methods & attributes for RTCStatsElementList
> 
> This comment isn't needed.

File gone.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:32
>> +class RTCStatsReport : public RefCounted<RTCStatsReport> {
> 
> I still don't see RTCStatsReport.cpp in this patch.  Is that what's causing the compile error?

It seems that I can get away with not implementing classes that I don't use when I'm using a component build, but not in a bot build. Implementations added.

[Harald Alvestrand]

Created attachment 165567 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14002697

[kov's GTK+ EWS bot]

Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14032005

[WebKit Review Bot]

Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14021053

[Harald Alvestrand]

Created attachment 165578 [details]
Patch - this version fixes buildbot-detected failures.

[Harald Alvestrand]

Created attachment 165582 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 165582 [details]
Patch

Attachment 165582 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14014011

[Harald Alvestrand]

Created attachment 165585 [details]
Patch

[Tommy Widenflycht]

I just built the gtk port locally and that works just fine. Have no idea why the bot fails :/

[Adam Barth]

> > Do you have a link to the spec handy?
> 
> Dated link: http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
> Should this be in the code somewhere? Perhaps just in the changelog?

ChangeLog is great.

[Adam Barth]

Comment on attachment 165585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165585&action=review

> Source/Platform/ChangeLog:1
> [phone]  Harald Tveit Alvestrand  <[e-mail]>

There's supposed to be a blank line after this line.

> Source/WebCore/ChangeLog:1
> [phone]  Harald Tveit Alvestrand  <[e-mail]>

here too

> Source/WebCore/Modules/mediastream/RTCStatsElement.cpp:38
> +    RefPtr<RTCStatsElement> response = adoptRef(new RTCStatsElement(timestamp));
> +    return response.release();

You can merge these two lines an eliminate the |response| variable.

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:37
> +    PassRefPtr<RTCStatsElement> create(long timestamp);

Shouldn't this function be static?

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:40
> +private:

Please add a blank line above this line.

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:41
> +    RTCStatsElement(long timestamp);

Almost all one-argument constructors should have the "explicit" keyword.

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:36
> +    RefPtr<RTCStatsReport> response = adoptRef(new RTCStatsReport());
> +    return response.release();

This can be merged into one line.

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:39
> +    virtual ~RTCStatsReport();

This function is declared by never implemented.  DOes this function need to be virtual?  Are you planning to subclass this class?

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:37
> +    RefPtr<RTCStatsResponse> response = adoptRef(new RTCStatsResponse());
> +    // FIXME: Pass an RTCStatsRequest argument and populate from it.
> +    return response.release();

I'd put this on one line and expand it in a subsequent patch, if needed.

> Tools/ChangeLog:1
> [phone]  Harald Tveit Alvestrand  <[e-mail]>

blank line below this line, pls

> Tools/ChangeLog:6
> +        http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html

Thanks for adding the link to the specification.  You don't need to put it in every ChangeLog.  Typically folks put it in the WebCore ChangeLog because that's where the bulk of the spec-implementing code is.

> Tools/ChangeLog:8
> +

Looks like you've got an extra blank line here.

[Adam Barth]

Comment on attachment 165585 [details]
Patch

This looks great.  Thanks for iterating on the patch.  It looks like we have a few minor style issues to clean up and then we'll be ready to land this patch.

[Harald Alvestrand]

Comment on attachment 165585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165585&action=review

>> Source/Platform/ChangeLog:1
>> [phone]  Harald Tveit Alvestrand  <[e-mail]>
> 
> There's supposed to be a blank line after this line.

Done.

>> Source/WebCore/ChangeLog:1
>> [phone]  Harald Tveit Alvestrand  <[e-mail]>
> 
> here too

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.cpp:38
>> +    return response.release();
> 
> You can merge these two lines an eliminate the |response| variable.

Done. Can always unmerge later.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:37
>> +    PassRefPtr<RTCStatsElement> create(long timestamp);
> 
> Shouldn't this function be static?

Oops. Yes.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:40
>> +private:
> 
> Please add a blank line above this line.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:41
>> +    RTCStatsElement(long timestamp);
> 
> Almost all one-argument constructors should have the "explicit" keyword.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:36
>> +    return response.release();
> 
> This can be merged into one line.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:39
>> +    virtual ~RTCStatsReport();
> 
> This function is declared by never implemented.  DOes this function need to be virtual?  Are you planning to subclass this class?

Deleted. Can always add it back if I need it.

>> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:37
>> +    return response.release();
> 
> I'd put this on one line and expand it in a subsequent patch, if needed.

Done.

>> Tools/ChangeLog:1
>> [phone]  Harald Tveit Alvestrand  <[e-mail]>
> 
> blank line below this line, pls

Done.

>> Tools/ChangeLog:6
>> +        http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
> 
> Thanks for adding the link to the specification.  You don't need to put it in every ChangeLog.  Typically folks put it in the WebCore ChangeLog because that's where the bulk of the spec-implementing code is.

OK, deleted from others.

>> Tools/ChangeLog:8
>> +
> 
> Looks like you've got an extra blank line here.

Fixed.

[Harald Alvestrand]

Created attachment 165644 [details]
Patch

[Adam Barth]

(In reply to comment #26)
> I just built the gtk port locally and that works just fine. Have no idea why the bot fails :/

DerivedSources/WebCore/JSRTCPeerConnection.cpp: In function 'void* WebCore::jsRTCPeerConnectionPrototypeFunctionGetStats(JSC::ExecState*)':
DerivedSources/WebCore/JSRTCPeerConnection.cpp:711:48: error: 'JSRTCStatsCallback' has not been declared

It does look like a failure related to this patch.  Does DerivedSources/WebCore/JSRTCPeerConnection.cpp #include the right stuff?

[Adam Barth]

Comment on attachment 165644 [details]
Patch

This looks good, but I worry that we're going to break the GTK port by landing this patch.

[Adam Barth]

Maybe we can ask someone who is more familiar with GTK to help us diagnose the build problem.  mrobinson, would you be willing to take a look or suggest someone who could?

[Harald Alvestrand]

Both Tommy and I have checked out fresh WebKit build environments and built the gtk port with the patch - and it compiles cleanly.

(checkout sources; apply patch; Tools/Scripts/build-webkit --gtk)

Definitely someone who understands how the gtk bot is different is going to have to look at this.

[Martin Robinson]

Comment on attachment 165644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165644&action=review

> Source/WebCore/GNUmakefile.list.am:649
> +	DerivedSources/WebCore/JSRTCStatsCallback.cpp \
> +	DerivedSources/WebCore/JSRTCStatsCallback.h \
> +	DerivedSources/WebCore/JSRTCStatsElement.cpp \
> +	DerivedSources/WebCore/JSRTCStatsElement.h \
> +	DerivedSources/WebCore/JSRTCStatsReport.cpp \
> +	DerivedSources/WebCore/JSRTCStatsReport.h \
> +	DerivedSources/WebCore/JSRTCStatsResponse.cpp \
> +	DerivedSources/WebCore/JSRTCStatsResponse.h \

If you add new IDLs, you should also add them to the dom_binding_idls in this file. That's probably why the build is broken in this case.

[Harald Alvestrand]

Created attachment 165791 [details]
Patch

[Adam Barth]

Comment on attachment 165791 [details]
Patch

Looks like the gtk bot is happy now.  Thanks!!

[WebKit Review Bot]

Comment on attachment 165791 [details]
Patch

Clearing flags on attachment: 165791

Committed r129654: <http://trac.webkit.org/changeset/129654>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 97702

[Harald Alvestrand]

Created attachment 166208 [details]
Patch

[Harald Alvestrand]

Comment on attachment 166208 [details]
Patch

Added one more #include.
This has been compiled with Chrome on Windows.

[WebKit Review Bot]

Comment on attachment 166208 [details]
Patch

Clearing flags on attachment: 166208

Committed r129908: <http://trac.webkit.org/changeset/129908>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.