[webkit-reviews] review canceled: [Bug 77087] [GStreamer] 0.11 video-sink : [Attachment 141630] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 23 10:50:34 PDT 2012


Philippe Normand <pnormand at igalia.com> has canceled Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 77087: [GStreamer] 0.11 video-sink
https://bugs.webkit.org/show_bug.cgi?id=77087

Attachment 141630: Patch
https://bugs.webkit.org/attachment.cgi?id=141630&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=141630&action=review


>> Source/WebCore/ChangeLog:10
>> +	    port my gst-mac WebKit branch over to WebKit2 mac port.
> 
> Do you mind splitting the CG removal into another patch? r+ in advance.

Sure!

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:57
>> +		return FloatRect(0, 0, -1, -1);
> 
> A comment here explaining these values might be good. Are these numbers that
Gstreamer expects?

These numbers are what the GraphicsContext draw method expect in the case where
there is no crop rectangle.

>>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1505

>> +		const gchar * const * extensions;
> 
> The asterisks should be attached to the typenames here.

Right. Anyway, this code is no longer necessary since the mimeTypeCache()
refactoring.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:187
>> +	if (!gst_video_frame_map(&frame, &priv->info, buffer, GST_MAP_READ)) {
> 
> I think it makes sense to split this off into a helper function like,
getFormatAndDimensions().

Well there is already, webkitGetVideoSizeAndFormatFromCaps but I couldn' t use
it with a GstVideoFrame.
I fully understand the motivation for these utility functions but IMHO they
restrain what the GStreamer APIs allow us to do. Will we need a utility
function for each way to get those information using gst APIs?

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:214
>>  
> 
> Again this would probably be better as a helper called, createBuffer.

Sure.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:233
>>	    guint8 *destination = GST_BUFFER_DATA(newBuffer);
> 
> The asterisks are in the wrong place here.

Indeed!

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:266
>> +#endif
> 
> Could you eliminate these calls by creating an OwnPtr for GstBuffer?

Hum.... I need to think about that and how to integrate this with GstMapInfo.


More information about the webkit-reviews mailing list