Bug 169727 - Improve WebRTC track enabled support
Summary: Improve WebRTC track enabled support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-15 17:36 PDT by youenn fablet
Modified: 2017-03-16 09:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (32.28 KB, patch)
2017-03-15 21:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.14 KB, patch)
2017-03-15 22:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (32.19 KB, patch)
2017-03-16 08:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-15 17:36:22 PDT
These tests could be improved a bit and made more clear why they fail.
Comment 1 youenn fablet 2017-03-15 21:59:36 PDT
Created attachment 304607 [details]
Patch
Comment 2 youenn fablet 2017-03-15 22:01:21 PDT
This is a first step.
If we follow that direction, we might want to handle that at RealtimeMediaSource level, so that it generates the black frame/silence in videoSampleAvailable/audioSamplesAvailable
Comment 3 youenn fablet 2017-03-15 22:02:14 PDT
Created attachment 304608 [details]
Patch
Comment 4 Alex Christensen 2017-03-16 00:34:03 PDT
Comment on attachment 304608 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Mkaking sure muted/disabled sources produce silence/black frames.

Making

> Source/WebCore/ChangeLog:24
> +        (WebCore::RealtimeIncomingVideoSource::pixelBufferFromVideoFrame):i Generating black frames if muted.

i

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSource.cpp:100
> +            auto status = CVPixelBufferCreate(kCFAllocatorDefault, frame.width(), frame.height(), kCVPixelFormatType_420YpCbCr8Planar, nullptr, &pixelBuffer);

This looks like a memory leak.  What deallocates this?  We should probably return a smart pointer type.

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSource.cpp:104
> +            ASSERT(!status);

Does status of 0 have a name?
Comment 5 youenn fablet 2017-03-16 08:28:54 PDT
Created attachment 304641 [details]
Patch for landing
Comment 6 youenn fablet 2017-03-16 08:32:17 PDT
Thanks for the review

(In reply to comment #4)
> Comment on attachment 304608 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304608&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Mkaking sure muted/disabled sources produce silence/black frames.
> 
> Making

OK

> 
> > Source/WebCore/ChangeLog:24
> > +        (WebCore::RealtimeIncomingVideoSource::pixelBufferFromVideoFrame):i Generating black frames if muted.
> 
> i

OK

> 
> > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSource.cpp:100
> > +            auto status = CVPixelBufferCreate(kCFAllocatorDefault, frame.width(), frame.height(), kCVPixelFormatType_420YpCbCr8Planar, nullptr, &pixelBuffer);
> 
> This looks like a memory leak.  What deallocates this?  We should probably
> return a smart pointer type.

pixelBuffer is retained by m_blackFrame.
I put the assignment closer to the allocation.


> 
> > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSource.cpp:104
> > +            ASSERT(!status);
> 
> Does status of 0 have a name?

Changed to noErr.
Comment 7 WebKit Commit Bot 2017-03-16 09:09:54 PDT
Comment on attachment 304641 [details]
Patch for landing

Clearing flags on attachment: 304641

Committed r214044: <http://trac.webkit.org/changeset/214044>
Comment 8 WebKit Commit Bot 2017-03-16 09:09:58 PDT
All reviewed patches have been landed.  Closing bug.