Bug 189262 – Implement MediaStreamTrack Content Hints

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Jon Lee]

Implement MediaStreamTrack Content Hints
https://w3c.github.io/mst-content-hint/

[Radar WebKit Bug Importer]

<rdar://problem/44101773>

[Wendy]

Created attachment 350083 [details]
Patch

[youenn fablet]

Comment on attachment 350083 [details]
Patch

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

> Source/WebCore/ChangeLog:30
> +        (WebCore::MediaStreamTrackPrivate::contentHint const):

Change log has double entries. Also it is marked "No new tests" but I see that you added some in this patch.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:100
> +bool MediaStreamTrack::setContentHint(const String& hintValue) const

Should probably be a String&& and use WTFMove().

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:73
> +    WEBCORE_EXPORT bool setContentHint(const String&) const;

Should not be const, no need for WEBCORE_EXPORT either.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:99
> +bool MediaStreamTrackPrivate::setContentHint(const String& hintValue)

String&&

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:103
> +            m_contentHint = hintValue;

m_contentHint = WTFMove(hintValue)
But, since contentHint is a limited set of values, we could use an enum maybe?

> LayoutTests/imported/w3c/ChangeLog:9
> +        * web-platform-tests/mediacapture-streams/MediaStreamTrack-contentHint.https.html: Added.

Are these new tests or tests imported from upstream WPT?

> LayoutTests/imported/w3c/ChangeLog:52
> +        https://bugs.webkit.org/show_bug.cgi?id=189262

This is probably the reason for the patch not applying.

[Wendy]

Created attachment 350291 [details]
Patch

[Wendy]

Created attachment 350292 [details]
Patch

[Jon Lee]

Comment on attachment 350292 [details]
Patch

This patch needs to be rebaselined on top of trunk for it to succeed on EWS.

[Wendy]

Created attachment 350407 [details]
Patch

[EWS Watchlist]

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


ERROR: LayoutTests/imported/w3c/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


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

[Wendy]

Created attachment 350459 [details]
Patch

[Wendy]

Created attachment 350460 [details]
Patch

[Wendy]

Created attachment 350484 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:71:  The parameter name "hintValue" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:105:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 8 files


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

[EWS Watchlist]

Comment on attachment 350484 [details]
Patch

Attachment 350484 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9307459

New failing tests:
imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html

[EWS Watchlist]

Created attachment 350490 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350484 [details]
Patch

Attachment 350484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9307483

New failing tests:
imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html

[EWS Watchlist]

Created attachment 350492 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Comment on attachment 350484 [details]
Patch

This patch is going in the right direction.
Some comments below.
-expected.txt file needs to be fixed.

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

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

Please explain that the changes are covered by rebased tests.
It would also be good to explain below your changes, for instance that contentHint is stored in MediaStreamTrackPrivate and add a reference to the spec.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:103
> +    MediaStreamTrackPrivate::HintValue val = m_private->contentHint();

You can use auto here instead of MediaStreamTrackPrivate::HintValue.
And replace val by value.
Given it is only used in the switch statement, I would simply write switch (m_private->contentHint())

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:104
> +    switch (val) {

According WebKit style, the case statement should be indented like the switch statement.
Please remove 4 spaces to all lines below.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:122
> +void MediaStreamTrack::setContentHint(const String&& hintValue)

s/&&/&/

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:124
> +    MediaStreamTrackPrivate::HintValue val;

s/val/value/

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:136
> +        val = MediaStreamTrackPrivate::HintValue::Text;

What happens if hintValue is not any of these value?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:99
> +bool MediaStreamTrackPrivate::setContentHint(HintValue hintValue)

bool value does not seem to be used anywhere.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:111
> +    }

I would move these checks in MediaStreamTrack::setContentHint and here it would just be a simple setter.

> LayoutTests/imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint-expected.txt:-4
> -FAIL Audio tracks ignore invalid/video contentHints assert_equals: Audio tracks should ignore video-only contentHints. expected "speech" but got "motion"

It seems bots do not agree with that expected.txt.

[Wendy]

Created attachment 350708 [details]
Patch

[youenn fablet]

Comment on attachment 350708 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by Youenn Fablet.

prepare-ChangeLog should have added OOPS here.

> Source/WebCore/ChangeLog:12
> +        New added Tests: 

s/New added Tests:/Covered by tests:/

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:123
> +    MediaStreamTrackPrivate::HintValue val;

s/val/value.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:136
> +    HintValue m_contentHint;

What is the default value for m_contentHint?
Do we have testing for that case?

[EWS Watchlist]

Comment on attachment 350708 [details]
Patch

Attachment 350708 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9337639

New failing tests:
imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html

[EWS Watchlist]

Created attachment 350720 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Wendy]

Created attachment 350727 [details]
Patch

[Wendy]

Created attachment 350761 [details]
Patch

[youenn fablet]

Comment on attachment 350761 [details]
Patch

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

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:70
> +    HintValue contentHint() { return m_contentHint; }

Would you be able to change it to a const getter as a follow-up patch?

[WebKit Commit Bot]

Comment on attachment 350761 [details]
Patch

Rejecting attachment 350761 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 350761, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9346960

[Eric Carlson]

Comment on attachment 350761 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by Youenn Fablet (OOPS!).

You should remove the "(OOPS!)"

[Chris Dumez]

Comment on attachment 350761 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +        Reviewed by Youenn Fablet (OOPS!).
> 
> You should remove the "(OOPS!)"

Reviewed by Youenn, OOPS indeed :D

[Wendy]

Created attachment 350785 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350785 [details]
Patch

Clearing flags on attachment: 350785

Committed r236478: <https://trac.webkit.org/changeset/236478>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.