815825 - Fake video streams should have content that changes over time

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P3)

Description

[Timothy B. Terriberry (:derf)]

This makes it more obvious that they are actually working and doing something.

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Rotate colors in fake video streams

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 685852 [details] [diff] [review]
Rotate colors in fake video streams

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

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +216,5 @@
> +  // Update the target color
> +  if (mCr <= 16)
> +  {
> +    if (mCb < 240)
> +    {

I think brace style for this file is "if (foo) {" and "} else {"

In any case, please match it

::: content/media/webrtc/MediaEngineDefault.h
@@ +63,5 @@
>    SourceMediaStream* mSource;
>    layers::PlanarYCbCrImage* mImage;
>    static const MediaEngineVideoOptions mOpts;
> +  int mCb;
> +  int mCr;

uint8_t?

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 685852 [details] [diff] [review]
Rotate colors in fake video streams

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

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +216,5 @@
> +  // Update the target color
> +  if (mCr <= 16)
> +  {
> +    if (mCb < 240)
> +    {

I will never get used to the fact that this is different for the function prototype vs. other blocks (when copying local style).

::: content/media/webrtc/MediaEngineDefault.h
@@ +63,5 @@
>    SourceMediaStream* mSource;
>    layers::PlanarYCbCrImage* mImage;
>    static const MediaEngineVideoOptions mOpts;
> +  int mCb;
> +  int mCr;

I think this should be kept as int, since we actually do arithmetic on it. If we had an array of a million of them, I would agree with you.

Comment 4

[Timothy B. Terriberry (:derf)]

Attachment: Rotate colors in fake video streams v2

Addressing comments from review, carrying forward r=jesup.

Comment 5

[Timothy B. Terriberry (:derf)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9634ee8c1cc

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

:derf, would it be possible to avoid black? Not sure if this is covered by your code but the spec says:

The output of a MediaStream object must correspond to the tracks in its input. Muted audio tracks must be replaced with silence. Muted video tracks must be replaced with blackness.

If we use blackness we could run into a race condition.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Tim's patch rotates the Cr and Cb (chroma components); it doesn't touch the luma (0x80, or mid-gray roughly).

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #7)
> Tim's patch rotates the Cr and Cb (chroma components); it doesn't touch the
> luma (0x80, or mid-gray roughly).

Right. It will never be black.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sounds good then. Thanks!

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9634ee8c1cc

Comment 11

[Jason Smith [:jsmith]]

in-testsuite+ mainly cause existing gum automation will test this flow I believe