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.
<rdar://problem/27379487>
Created attachment 283832 [details] Patch
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 > +2016-07-14 George Ruan <gruan@apple.com> > + > + 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.
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
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
Created attachment 283920 [details] Patch
Created attachment 283932 [details] EWS
Created attachment 283933 [details] Patch
Created attachment 283934 [details] EWS
Created attachment 283969 [details] Patch
Created attachment 283978 [details] Patch
Comment on attachment 283978 [details] Patch Clearing flags on attachment: 283978 Committed r203423: <http://trac.webkit.org/changeset/203423>
All reviewed patches have been landed. Closing bug.
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
Re-opened since this is blocked by bug 159977
Created attachment 284279 [details] Patch
Created attachment 284293 [details] Patch
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.
Created attachment 284351 [details] Patch
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.
Created attachment 284393 [details] Patch
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
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
Created attachment 284623 [details] Patch
Comment on attachment 284623 [details] Patch Clearing flags on attachment: 284623 Committed r203739: <http://trac.webkit.org/changeset/203739>