848189 - Fake Video Implementation for WebRTC Tests

Status: REOPENED

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

Description

[Suhas]

WebRTC Unit tests and echo-server project needs Fake Video implementation.

Comment 1

[Suhas]

Attachment: Fake Video Implementation for WebRTC Tests

Comment 2

[Suhas]

Comment on attachment 721574 [details]
Fake Video Implementation for WebRTC Tests

Patch that does fake video verification for signaling unit tests. Have tested it on OSX locally. Will verify it on Linux tomorrow morning.

Comment 3

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

Comment on attachment 721574 [details]
Fake Video Implementation for WebRTC Tests

Review of attachment 721574 [details]:
-----------------------------------------------------------------

Waiting on r+/- until the SYNC issue is answered

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +649,5 @@
> +      Fake_AudioStreamSource *audio_stream =
> +                              new Fake_AudioStreamSource();
> +      test_utils->sts_target()->Dispatch(
> +        WrapRunnableRet(audio_stream, &Fake_MediaStream::Start, &ret),
> +        NS_DISPATCH_SYNC);

Do these need to be DISPATCH_SYNC?

Can ::Start() in Fake_MediaStrema actually fail in any reasonable fashion?  And if so, is it important that that be noticed here - can we just fail the test in ::Start()?

That said, I'm not certain the restrictions against DISPATCH_SYNC are really relevant in the unit tests today - but if we can move them to the "real" mainthread, perhaps they will be (haven't thought closely about it).

@@ +663,5 @@
> +
> +      test_utils->sts_target()->Dispatch(
> +        WrapRunnableRet(video_stream, &Fake_MediaStream::Start, &ret),
> +        NS_DISPATCH_SYNC);
> +      ASSERT_TRUE(NS_SUCCEEDED(ret));

ditto

Comment 4

[Suhas]

Comment on attachment 721574 [details]
Fake Video Implementation for WebRTC Tests

Review of attachment 721574 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +649,5 @@
> +      Fake_AudioStreamSource *audio_stream =
> +                              new Fake_AudioStreamSource();
> +      test_utils->sts_target()->Dispatch(
> +        WrapRunnableRet(audio_stream, &Fake_MediaStream::Start, &ret),
> +        NS_DISPATCH_SYNC);

Since this is the way the unittests are run today, should we go with DISPATCH_SYNC for now and open a new bug to fix it once the current DISPATHC_SYNC issue is resolved.

Comment 5

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

Comment on attachment 721574 [details]
Fake Video Implementation for WebRTC Tests

Review of attachment 721574 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +140,5 @@
> +                    mozilla::TrackTicks aDuration,
> +                    const Fake_gfxIntSize& aIntrinsicSize);
> +
> +  const Fake_VideoFrame* GetFrameAt(mozilla::TrackTicks aOffset,
> +                                      mozilla::TrackTicks* aStart = nullptr)

indentation - check you don't have tabs; I see this in heavily-indented function devs in this

::: media/webrtc/signaling/test/FakeMediaStreamsImpl.h
@@ +101,5 @@
>  }
>  
>  
> +// Fake_VideoFrame
> +Fake_VideoFrame::Fake_VideoFrame(already_AddRefed<Fake_Image> aImage,const Fake_gfxIntSize& aIntrinsicSize)

space after comma (throughout)

@@ +105,5 @@
> +Fake_VideoFrame::Fake_VideoFrame(already_AddRefed<Fake_Image> aImage,const Fake_gfxIntSize& aIntrinsicSize)
> +    :mImage(aImage),mIntrinsicSize(aIntrinsicSize) {}
> +
> +
> +Fake_VideoFrame::Fake_VideoFrame():mIntrinsicSize(0, 0) {}

spaces around ':'

@@ +157,5 @@
> +    return;
> +  }
> +
> +  nsRefPtr<Fake_Image> image = new Fake_Image();
> +  memset(image->mData.get(),IMAGE_COLOR,image->YUVSize());

You may want to check the current "video:true fake:true" generation that does a slow color shift so you can see if things are alive.

@@ +167,5 @@
> +  segment.AppendFrame(image.forget(), 0, Fake_gfxIntSize(width,height));
> +
> +  for(std::set<Fake_MediaStreamListener *>::iterator it = mListeners.begin();
> +       it != mListeners.end(); ++it) {
> +    (*it)->NotifyQueuedTrackChanges(NULL, // Graph

NULL for graph could cause real problems, but current users appear to not use it.  At least comment that if anyone starts using it, at least a dumb impl of the interface will be required.

@@ +169,5 @@
> +  for(std::set<Fake_MediaStreamListener *>::iterator it = mListeners.begin();
> +       it != mListeners.end(); ++it) {
> +    (*it)->NotifyQueuedTrackChanges(NULL, // Graph
> +                                    0, // TrackID
> +                                    90000, // Rate (hz)

USECS_PER_S (1000000)

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +659,4 @@
>      if (offerFlags & OFFER_VIDEO) {
>        aHintContents |= DOMMediaStream::HINT_CONTENTS_VIDEO;
> +      Fake_VideoStreamSource *video_stream =
> +                              new Fake_VideoStreamSource();

one line

Comment 6

[Suhas]

Comment on attachment 721574 [details]
Fake Video Implementation for WebRTC Tests

MediaPipeline, Thread Dispatch code has changed a lot since this patch was reviewed. Hence making this patch obsolete and uploading new ones.

Comment 7

[Suhas]

Attachment: Part3 Enable VideoCall tests in signalingtests

Signaling Video call test to verify FakeVideo changes after Dispatch Sync landed.

Comment 8

[Suhas]

Attachment: Part2 MediaPipeline updates to support Fake_Video

MediaPipeline with FakeVideo support with changes from DispatchSync bug landed.

Comment 9

[Suhas]

Attachment: Part1 FakeMediaStreams* to support Fake_Video

FakeVideo Implementation

Comment 10

[Suhas]

Taking r+ from Randell forward.
These new patches are breakup of a one large patch for FakeVideo support and these are updated to reflect changes from Disptach sync family of updates.

Comment 11

[Ethan Hugg [:ehugg]]

Comment on attachment 731605 [details] [diff] [review]
Part1  FakeMediaStreams* to support Fake_Video

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

r+ with comments addressed.

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +70,5 @@
> +    return height;
> +  }
> +
> +  int width;
> +  int height;

I would expect these to be mWidth or width_ and probably be private as well since there are accessor methods above

@@ +88,5 @@
> +    return (mImage == aFrame.mImage);
> +  }
> +
> +  bool operator!=(const Fake_VideoFrame& aFrame) const {
> +    return !operator==(aFrame);

Is this where you intended to put the parens on this line?

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 731603 [details] [diff] [review]
Part3 Enable VideoCall tests in signalingtests

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

r+ with comment addressed.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +923,1 @@
>    source->AsSourceStream()->AddTrack(track_id, track_rate, 0, segment);

So, my current source already has the AddListener line and I don't understand this comment.  Which is the 'below change'?

Comment 13

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #12)
> Comment on attachment 731603 [details] [diff] [review]
> Part3 Enable VideoCall tests in signalingtests
> 
> Review of attachment 731603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comment addressed.
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +923,1 @@
> >    source->AsSourceStream()->AddTrack(track_id, track_rate, 0, segment);
> 
> So, my current source already has the AddListener line and I don't
> understand this comment.  Which is the 'below change'?
Adding listener didn't reflect in the latest version of m-c when i uploaded this patch. The plan is to remove that line from this patch before committing.

Comment 14

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #11)
> Comment on attachment 731605 [details] [diff] [review]
> Part1  FakeMediaStreams* to support Fake_Video
> 
> Review of attachment 731605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: media/webrtc/signaling/test/FakeMediaStreams.h
> @@ +70,5 @@
> > +    return height;
> > +  }
> > +
> > +  int width;
> > +  int height;
> 
> I would expect these to be mWidth or width_ and probably be private as well
> since there are accessor methods above
> 
> @@ +88,5 @@
> > +    return (mImage == aFrame.mImage);
> > +  }
> > +
> > +  bool operator!=(const Fake_VideoFrame& aFrame) const {
> > +    return !operator==(aFrame);
> 
> Is this where you intended to put the parens on this line?

it was supposed to be negation of operator==(). Am i missing something here ..

Comment 15

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

Comment on attachment 731605 [details] [diff] [review]
Part1  FakeMediaStreams* to support Fake_Video

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

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +50,5 @@
> +class Fake_Image {
> + public:
> +  Fake_Image(): width(640),
> +                height(480),
> +                mData(new uint8_t[(width*height)* 3/2]) {}

Use YUVSize()

@@ +72,5 @@
> +
> +  int width;
> +  int height;
> +  nsAutoPtr<uint8_t> mData;
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Fake_Image);

Normal style is to put DECL macros at/near the top

@@ +88,5 @@
> +    return (mImage == aFrame.mImage);
> +  }
> +
> +  bool operator!=(const Fake_VideoFrame& aFrame) const {
> +    return !operator==(aFrame);

Parens for clarity

@@ +109,5 @@
> +
> +  void SliceTo(mozilla::TrackTicks aStart, mozilla::TrackTicks aEnd)
> +  {
> +    NS_ASSERTION(aStart >= 0 && aStart < aEnd && aEnd <= mDuration,
> +                 "Slice out of bounds");

MOZ_ASSERT please

@@ +281,5 @@
> +      while(!iter.IsEnded()) {
> +        Fake_VideoChunk& chunk = *(iter);
> +        Fake_Image *img = chunk.mFrame.GetImage();
> +        if(!img)
> +        {

brace style

@@ +286,5 @@
> +          return true;
> +        }
> +        uint8_t* buf = img->mData.get();
> +        //TODO: remove hard-coding
> +        int size = ((640*480) * 3 / 2);

If hard-coded, use #defines for width/height (and share with code that creates the images).
Use YUVSize()

@@ +431,5 @@
> +  }
> +
> +  virtual void Periodic();
> +  bool mStop;
> +  nsRefPtr<Fake_Image> mSourceImage;

Should these be private?  (Not a big deal)

Comment 16

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

Comment on attachment 731604 [details] [diff] [review]
Part2 MediaPipeline updates to support Fake_Video

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +824,5 @@
> +#endif
> +#ifdef USE_FAKE_MEDIA_STREAMS
> +    uint32_t width = img->Width();
> +    uint32_t height = img->Height();
> +    int length = ((width * height) * 3 / 2);

Can we use YUVSize() here?

Comment 17

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

Comment on attachment 731603 [details] [diff] [review]
Part3 Enable VideoCall tests in signalingtests

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

r+ modulo answering ehugg's question

Comment 18

[Suhas]

Comment on attachment 731603 [details] [diff] [review]
Part3 Enable VideoCall tests in signalingtests

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +923,1 @@
>    source->AsSourceStream()->AddTrack(track_id, track_rate, 0, segment);

since this patch was written before the above line was added to the MediaPipeline code. The comment meant to remove this line in the final patch.

Comment 19

[Suhas]

Comment on attachment 731604 [details] [diff] [review]
Part2 MediaPipeline updates to support Fake_Video

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +824,5 @@
> +#endif
> +#ifdef USE_FAKE_MEDIA_STREAMS
> +    uint32_t width = img->Width();
> +    uint32_t height = img->Height();
> +    int length = ((width * height) * 3 / 2);

Sure Randell .. I will investigate this.

Comment 20

[Suhas]

Comment on attachment 731605 [details] [diff] [review]
Part1  FakeMediaStreams* to support Fake_Video

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

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +70,5 @@
> +    return height;
> +  }
> +
> +  int width;
> +  int height;

Sure Ethan .. will do

@@ +72,5 @@
> +
> +  int width;
> +  int height;
> +  nsAutoPtr<uint8_t> mData;
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Fake_Image);

I will update it as suggested

@@ +88,5 @@
> +    return (mImage == aFrame.mImage);
> +  }
> +
> +  bool operator!=(const Fake_VideoFrame& aFrame) const {
> +    return !operator==(aFrame);

I will do so

@@ +109,5 @@
> +
> +  void SliceTo(mozilla::TrackTicks aStart, mozilla::TrackTicks aEnd)
> +  {
> +    NS_ASSERTION(aStart >= 0 && aStart < aEnd && aEnd <= mDuration,
> +                 "Slice out of bounds");

Agreed .. Will update it

@@ +281,5 @@
> +      while(!iter.IsEnded()) {
> +        Fake_VideoChunk& chunk = *(iter);
> +        Fake_Image *img = chunk.mFrame.GetImage();
> +        if(!img)
> +        {

Will change

@@ +286,5 @@
> +          return true;
> +        }
> +        uint8_t* buf = img->mData.get();
> +        //TODO: remove hard-coding
> +        int size = ((640*480) * 3 / 2);

Sure

@@ +431,5 @@
> +  }
> +
> +  virtual void Periodic();
> +  bool mStop;
> +  nsRefPtr<Fake_Image> mSourceImage;

Will make them so

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 731605 [details] [diff] [review]
Part1  FakeMediaStreams* to support Fake_Video

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

Is this still relevant? If so, please r? again.

Comment 22

[Eric Rescorla (:ekr)]

Comment on attachment 731604 [details] [diff] [review]
Part2 MediaPipeline updates to support Fake_Video

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

Is this still relevant? If so, please r? again.

Comment 23

[Eric Rescorla (:ekr)]

Comment on attachment 731603 [details] [diff] [review]
Part3 Enable VideoCall tests in signalingtests

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

Is this still relevant? If so, please r? again.

Comment 24

[Dan Minor [:dminor]]

I'm going to assume this is no longer relevant at this point given the age of the patches.

Comment 25

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

(In reply to Dan Minor [:dminor] from comment #24)
> I'm going to assume this is no longer relevant at this point given the age
> of the patches.

The patches may be pretty dusty/rotted, but last I checked we didn't have real video support in the unit tests.  See all the "if !defined(MOZILLA_EXTERNAL_LINKAGE)" lines in MediaPipeline, for example.  So the bug/ask to include video in unit tests is still there.

Comment 26

[Dan Minor [:dminor]]

We'll need to fix this as part of Bug 1319489.

Comment 27

[Dan Minor [:dminor]]

As it turns out, Bug 1319489 only needs a fake audio implementation.

Comment 28

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.