Bug 159833 – HTMLVideoElement frames do not update on iOS when src is a MediaStream blob

• Component: Media
• Status: RESOLVED FIXED

[George Ruan]

HTMLVideoElement frames do not update on iOS when src is a MediaStream blob.

MediaPlayerPrivateMediaStream should use a displayLayer instead of a CALayer to show sampleBuffers from the AVVideoCaptureSource.

[Radar WebKit Bug Importer]

<rdar://problem/27379487>

[George Ruan]

Created attachment 283832 [details]
Patch

[Eric Carlson]

Comment on attachment 283832 [details]
Patch

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

> Source/WebCore/ChangeLog:45
> +        * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::~MediaPlayerPrivateMediaStreamAVFObjC):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::isAvailable):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::enqueueAudioSampleBufferFromTrack):
> +        Placeholder for future patch.
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::enqueueVideoSampleBufferFromTrack):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::ensureLayer):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::destroyLayer):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::platformLayer):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::currentDisplayMode):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::sampleBufferUpdated):
> +        (WebCore::updateTracksOfType):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::updateTracks):
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::acceleratedRenderingStateChanged):
> +        * platform/mediastream/MediaStreamPrivate.cpp:
> +        (WebCore::MediaStreamPrivate::updateActiveVideoTrack):
> +        * platform/mediastream/MediaStreamTrackPrivate.cpp:
> +        (WebCore::MediaStreamTrackPrivate::sourceHasMoreMediaData):
> +        * platform/mediastream/MediaStreamTrackPrivate.h:
> +        (WebCore::MediaStreamTrackPrivate::Observer::sampleBufferUpdated):
> +        (WebCore::MediaStreamTrackPrivate::setId):
> +        * platform/mediastream/RealtimeMediaSource.cpp:
> +        (WebCore::RealtimeMediaSource::mediaDataUpdated):
> +        * platform/mediastream/RealtimeMediaSource.h:
> +        * platform/mediastream/mac/AVVideoCaptureSource.mm:
> +        (WebCore::AVVideoCaptureSource::processNewFrame):

I think it is helpful to have a brief note about what changed in each method. Not everyone does this, but I think it makes it much easier for someone to later figure out what was added/changed.

> Source/WebCore/ChangeLog:86
> [phone]  George Ruan  <[e-mail]>
> +
> +        Move MediaSampleAVFObjC into its own file
> +        https://bugs.webkit.org/show_bug.cgi?id=159796
> +        <rdar://problem/27362488>
> +
> +        In preparation for a feature that uses MediaSampleAVFObjC, but does
> +        not need SourceBufferPrivateAVFObjC, it is beneficial to move
> +        MediaSampleAVFObjC to its own file.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * platform/MediaSample.h: Allow setting trackID to associate
> +        MediaSample id with MediaStreamTrackPrivate id.
> +        * platform/graphics/avfoundation/MediaSampleAVFObjC.h: Added.
> +        * platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm: Moved
> +        from MediaSampleAVFObjC
> +        (WebCore::MediaSampleAVFObjC::presentationTime):
> +        (WebCore::MediaSampleAVFObjC::decodeTime):
> +        (WebCore::MediaSampleAVFObjC::duration):
> +        (WebCore::MediaSampleAVFObjC::sizeInBytes):
> +        (WebCore::MediaSampleAVFObjC::platformSample):
> +        (WebCore::CMSampleBufferIsRandomAccess):
> +        (WebCore::MediaSampleAVFObjC::flags):
> +        (WebCore::MediaSampleAVFObjC::presentationSize):
> +        (WebCore::MediaSampleAVFObjC::dump):
> +        (WebCore::MediaSampleAVFObjC::offsetTimestampsBy):
> +        (WebCore::MediaSampleAVFObjC::setTimestamps):
> +        * platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
> +        Moved MediaSampleAVFObjC to its own file.
> +        (WebCore::MediaSampleAVFObjC::platformSample): Deleted.
> +        (WebCore::CMSampleBufferIsRandomAccess): Deleted.
> +        (WebCore::MediaSampleAVFObjC::flags): Deleted.
> +        (WebCore::MediaSampleAVFObjC::presentationSize): Deleted.
> +        (WebCore::MediaSampleAVFObjC::dump): Deleted.
> +        (WebCore::MediaSampleAVFObjC::offsetTimestampsBy): Deleted.
> +        (WebCore::MediaSampleAVFObjC::setTimestamps): Deleted.
> +        * platform/mock/mediasource/MockSourceBufferPrivate.cpp:
> +

This doesn't belong in this patch.

> Source/WebCore/platform/MediaSample.h:35
>  
> +typedef struct opaqueCMSampleBuffer *CMSampleBufferRef;
> +

Or the changes in this file.

> Source/WebCore/platform/graphics/avfoundation/MediaSampleAVFObjC.h:28
> +#pragma once
> +
> +#include "MediaSample.h"

Or this

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:126
> +    void enqueueAudioSampleBufferFromTrack();
> +    void enqueueVideoSampleBufferFromTrack(MediaStreamTrackPrivate&, PlatformSample);

It is very likely that enqueueAudioSampleBufferFromTrack will need the same parameters as enqueueVideoSampleBufferFromTrack, so you might as well add them now.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:64
> +
> +@interface AVSampleBufferDisplayLayer : CALayer
> +@property (readonly, getter=isReadyForMoreMediaData) BOOL readyForMoreMediaData;
> +- (void)enqueueSampleBuffer:(CMSampleBufferRef)sampleBuffer;
> +- (void)flush;
> +@end

This should go into a new SPI.h file so we use the real header when building with an internal SDK. It will be used on both iOS and Mac, so I think it should probably be WebCore/platform/spi/cocoa/AVSampleBufferDisplayLayerSPI.h. Look at one of the exiting files in that directory for an example.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:132
> +    // FIXME: <rdar://problem/27380390>

This should reference a bugs.webkit.org bug.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:138
> +    if (&track != m_mediaStreamPrivate->activeVideoTrack())
> +        return;

This should probably ASSERT in a debug build.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:151
> +    [m_sampleBufferDisplayLayer setName:@"MediaPlayerPrivateStream AVSampleBufferDisplayLayer"];

Nit: "MediaPlayerPrivateStream" -> "MediaPlayerPrivateMediaStreamAVFObjC"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:157
> +    // FIXME: move this somewhere appropriate:
> +    m_player->firstVideoFrameAvailable();

This should happen the first time a frame is enqueued.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:470
> +    if (platformSample.type != PlatformSample::CMSampleBufferType)
> +        return;

This should probably ASSERT in a debug build.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:533
> +        track->streamTrack()->removeObserver(*trackObserver);

Nice!

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:5
> + * Copyright (C) 2016 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions

Oops!

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:40
>  #import "MediaDescription.h"
>  #import "MediaPlayerPrivateMediaSourceAVFObjC.h"
>  #import "MediaSample.h"
> +#import "MediaSampleAVFObjC.h"
>  #import "MediaSourcePrivateAVFObjC.h"

Ditto

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:260
> -        if (!track->ended() && track->type() == RealtimeMediaSource::Type::Video && !track->ended()) {
> +        if (!track->ended() && track->type() == RealtimeMediaSource::Type::Video) {

Is this change correct?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:54
> +        virtual void sampleBufferUpdated(MediaStreamTrackPrivate&, RefPtr<MediaSample>) { };

I think this should parameter should probably be a MediaSample& because the WebKit RefPtr documentation (https://webkit.org/blog/5381/refptr-basics/) now says this about function arguments: "If a function does not take ownership of an object, the argument should be a raw reference or raw pointer."

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:110
> +    void sourceHasMoreMediaData(RefPtr<MediaSample>) final;

Ditto.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:72
> +        virtual void sourceHasMoreMediaData(RefPtr<MediaSample>) = 0;

Ditto.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:94
> +    void mediaDataUpdated(RefPtr<MediaSample>);

It's turtles all the way down!

> LayoutTests/fast/mediastream/MediaStream-video-element-displays-buffer.html:16
> +    var mediaStream;
> +    var video;
> +    var canvas;
> +    var context;

"let" and "const" are the new hotness!

> LayoutTests/fast/mediastream/MediaStream-video-element-displays-buffer.html:28
> +    function isPixelTransparent()
> +    {
> +        return pixel[0] === 0 && pixel[1] === 0 && pixel[2] === 0 && pixel[3] === 0;
> +    }
> +
> +    function isPixelBlack()
> +    {
> +        return pixel[0] === 255 && pixel[1] === 255 && pixel[2] === 255 && pixel[3] === 255;
> +    }

Can you pass the pixel to be tested as a parameter instead of referencing the global variable?

> LayoutTests/fast/mediastream/MediaStream-video-element-displays-buffer.html:34
> +        context.clearRect(0, 0, canvas.width, canvas.height);

It would be useful to test that the pixel is transparent at this point.

[Build Bot]

Comment on attachment 283832 [details]
Patch

Attachment 283832 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1688986

New failing tests:
fast/mediastream/MediaStream-video-element-displays-buffer.html

[Build Bot]

Created attachment 283837 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[George Ruan]

Created attachment 283920 [details]
Patch

[George Ruan]

Created attachment 283932 [details]
EWS

[George Ruan]

Created attachment 283933 [details]
Patch

[George Ruan]

Created attachment 283934 [details]
EWS

[George Ruan]

Created attachment 283969 [details]
Patch

[George Ruan]

Created attachment 283978 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283978 [details]
Patch

Clearing flags on attachment: 283978

Committed r203423: <http://trac.webkit.org/changeset/203423>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The test added with this change is frequently failing on El Capitan and Yosemite Release WK2

Link to failure:
https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r203451%20(7806)/results.html

Flakiness dashboard:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fmediastream%2FMediaStream-video-element-displays-buffer.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159977

[George Ruan]

Created attachment 284279 [details]
Patch

[George Ruan]

Created attachment 284293 [details]
Patch

[Eric Carlson]

Comment on attachment 284293 [details]
Patch

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

r=me if you move the Apple-specific stuff into MockRealtimeVideoSourceMac.

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:340
> +    RetainPtr<CVPixelBufferRef> pixelBuffer = pixelBufferFromCGImage(imageBuffer()->copyImage()->getCGImageRef());
> +    RetainPtr<CMSampleBufferRef> sampleBuffer = CMSampleBufferFromPixelBuffer(pixelBuffer.get());
> +
> +    mediaDataUpdated(MediaSampleAVFObjC::create(sampleBuffer.get()));
> +

This is platform specific so it shouldn't be in the base class. Why don't you put it in MockRealtimeVideoSourceMac::updatePlatformLayer?

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:344
> +RetainPtr<CMSampleBufferRef> MockRealtimeVideoSource::CMSampleBufferFromPixelBuffer(CVPixelBufferRef pixelBuffer)

This method is also platform specific, I would just put it in MockRealtimeVideoSourceMac

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:366
> +    CMSampleBufferCreateReadyWithImageBuffer(kCFAllocatorDefault, (CVImageBufferRef)pixelBuffer, formatDescription, &timingInfo, &sampleBuffer);
> +    CFRelease(formatDescription);
> +    if (status != noErr)

Don't you mean "status = CMSampleBufferCreateReadyWithImageBuffer(...)" ?

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:367
> +        LOG_ERROR("Failed to initialize CMSampleBuffer with error code: %d", status);

If the sample buffer creation fails, shouldn't return nullptr?

> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:39
> +#include <CoreVideo/CVPixelBuffer.h>

This Apple header shouldn't be included in this cross platform file.

> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:64
> +    virtual RetainPtr<CVPixelBufferRef> pixelBufferFromCGImage(CGImageRef) { return nullptr; };

A CVPixelBuffer and CGImageRef are both Apple specific so they shouldn't be included in this cross platform file.

> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:86
> +    RetainPtr<CMSampleBufferRef> CMSampleBufferFromPixelBuffer(CVPixelBufferRef);

Ditto. I would just put this in the derived class.

[George Ruan]

Created attachment 284351 [details]
Patch

[Darin Adler]

Comment on attachment 284351 [details]
Patch

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

A few comments, even though the patch is already reviewed.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:56
> +class MediaPlayerPrivateMediaStreamAVFObjC : public MediaPlayerPrivateInterface, public MediaStreamPrivate::Observer, public MediaStreamTrackPrivate::Observer {

Can this be private or protected derivation from any of these bases instead of public?

Can this class be marked final?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:490
> +void updateTracksOfType(HashMap<String, RefT>& trackMap, RealtimeMediaSource::Type trackType, MediaStreamTrackPrivateVector& currentTracks, RefT (*itemFactory)(MediaStreamTrackPrivate&), MediaPlayer* player, void (MediaPlayer::*removedFunction)(PassRefT), void (MediaPlayer::*addedFunction)(PassRefT), std::function<void(RefT, int)> configureCallback, MediaPlayerPrivateMediaStreamAVFObjC* trackObserver)

Can this new argument be a reference instead of a pointer?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:302
> +        settingsDidChanged();

Might be worth fixing the bad grammar in the function name here. It’s not "settings did changed".

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.h:38
> +#include <CoreVideo/CVPixelBuffer.h>

Can we forward declare the types instead of including the header?

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:47
> +#import "CoreMediaSoftLink.h"
> +#import "CoreVideoSoftLink.h"

These should be sorted in with the other imports above, not in their own paragraph.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:66
> +    OSStatus status = noErr;

Why not declare this at first use. There is also no reason to initialize it before overwriting it.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:67
> +    CMSampleBufferRef sampleBuffer;

Why not declare this below right where it is used, rather than above this whole paragraph.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:99
> +    CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault, frameSize.width, frameSize.height,  kCVPixelFormatType_32ARGB, (__bridge CFDictionaryRef) options, &pixelBuffer);

There’s an extra stray space in here before kCVPixelFormatType_32ARGB.

Seems like we could do the cast for "options" where we define it rather than here in the function call.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:106
> +    RetainPtr<CGColorSpaceRef> rgbColorSpace = adoptCF(CGColorSpaceCreateDeviceRGB());
> +    RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(data, frameSize.width, frameSize.height, 8, CVPixelBufferGetBytesPerRow(pixelBuffer), rgbColorSpace.get(), (CGBitmapInfo) kCGImageAlphaNoneSkipFirst));

It would be better to use auto here instead of writing out the types, I think.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:159
> +    RetainPtr<CVPixelBufferRef> pixelBuffer = pixelBufferFromCGImage(imageBuffer()->copyImage()->getCGImageRef());
> +    RetainPtr<CMSampleBufferRef> sampleBuffer = CMSampleBufferFromPixelBuffer(pixelBuffer.get());

It would be better to use auto here instead of writing out the types, I think.

[George Ruan]

Created attachment 284393 [details]
Patch

[Build Bot]

Comment on attachment 284393 [details]
Patch

Attachment 284393 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1737004

New failing tests:
fast/mediastream/MediaStream-video-element-displays-buffer.html

[Build Bot]

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

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

[George Ruan]

Created attachment 284623 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284623 [details]
Patch

Clearing flags on attachment: 284623

Committed r203739: <http://trac.webkit.org/changeset/203739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.