[Webkit-unassigned] [Bug 117039] [GStreamer] Support audio and video tracks

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117039





--- Comment #34 from Philippe Normand <pnormand at igalia.com>  2013-06-24 07:11:52 PST ---
(From update of attachment 205093)
View in context: https://bugs.webkit.org/attachment.cgi?id=205093&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please fix this line, remove OOPS, I suppose existing tests cover this change?

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

You can mention yourself here or the company you work for :) Unless Apple hired you? About the header below, please make sure this is really the one you want to use.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:88
> +    g_signal_handlers_disconnect_by_func(m_pad.get(),
> +        reinterpret_cast<gpointer>(audioTrackPrivateMuteChangedCallback), this);
> +    g_signal_handlers_disconnect_by_func(m_pad.get(),
> +        reinterpret_cast<gpointer>(audioTrackPrivateTagsChangedCallback), this);

Not sure indenting is needed here unless the line is wider than 120 characters. Same for various parts of the patch below.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:136
> +    gchar* str;

tagValue?

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:79
> +#endif // ENABLE(VIDEO) && USE(AVFOUNDATION) && HAVE(AVFOUNDATION_TEXT_TRACK_SUPPORT)

C&P error here I suppose :P

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:258
> +        m_audioTracks[i]->disconnect();

Hum, isn't the Vector dtor supposed to do its job here? If the disconnect method is used only here it can be merged to the Track dtor.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:261
> +        m_videoTracks[i]->disconnect();

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:606
> +        if (i < (gint)m_videoTracks.size()) {

I don't think a cast is needed here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:618
> +    while ((gint)m_videoTracks.size() > numTracks) {

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:621
> +        track->disconnect();
> +        m_videoTracks.removeLast();

Shouldn't removeLast() calls the track dtor?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:654
> +    for (gint i = 0; i < numTracks; ++i) {

Well this code is quite similar to the video tracks code above, so same questions :)

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.h:79
> +#endif // ENABLE(VIDEO) && USE(AVFOUNDATION) && HAVE(AVFOUNDATION_TEXT_TRACK_SUPPORT)

Another C&P error. The comments on AudioTrackPrivateGStreamer also apply to this module since the code is similar.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.