In the debug messages at least it would be good to have readable codes. Not sure which gst function to use though: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gsttaglanguagecodes.html
Which video were you looking at that had weird language codes?
(In reply to comment #1) > Which video were you looking at that had weird language codes? The video of that dash demo I mentioned on IRC
I have a patch to run language codes through gst_tag_get_language_code_iso_639_1, but I'm not sure of a reasonable way to test this. :\
Created attachment 217304 [details] Patch
Another thing we could do is run languages through gst_tag_get_language_name for debug messages, but it seems pretty wasteful to do that if we don't know that anyone is looking at debug messages. Is there a way to check if debugging it turned on?
Comment on attachment 217304 [details] Patch Attachment 217304 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/27138158
Comment on attachment 217304 [details] Patch Attachment 217304 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/27118129
(In reply to comment #5) > Another thing we could do is run languages through gst_tag_get_language_name for debug messages, but it seems pretty wasteful to do that if we don't know that anyone is looking at debug messages. Is there a way to check if debugging it turned on? gst_debug_is_active() perhaps? Another, ugly, solution would be to check if the GST_DEBUG env var is set.
Comment on attachment 217304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217304&action=review This looks good to me, sorry for the late review, heh. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). You'll want to remove this line
Created attachment 225942 [details] Patch
I also simplified the if (client) check to only be done once..
Comment on attachment 225942 [details] Patch Clearing flags on attachment: 225942 Committed r165175: <http://trac.webkit.org/changeset/165175>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 129788
Watch out for the EWSs, they don't post comments about the failures anymore. This is fixed by adding the gstreamer-tag-1.0 dependency to the pkg-config check, but I rolled it out so it would be cleared whether introducing the new dependency is OK.
Created attachment 226026 [details] Patch I'm nto test this, since the build works for me without this change (with and without gst-git). As for whether adding gstreamer-tag is a good idea: It's part of -base, which we're already using, and we'll need gsttag to get the track id and kind, once GStreamer accepts my patches.
(In reply to comment #16) > I'm nto test this, "I'm not able to test this"
Hm not sure why CMake doesn't like this. I'll spend more time trying to get the CMake build to work for me.
I'm going to check it on the GTK CMake build
Created attachment 226098 [details] Patch
Attachment 226098 [details] did not pass style-queue: ERROR: Source/WebCore/PlatformGTK.cmake:380: There should be exactly one empty line instead of 0 between "Modules/geolocation/Geolocation.idl" and "Modules/mediasource/VideoPlaybackQuality.idl". [list/emptyline] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 226098 [details] Patch This is not mine :) Only fixed it for CMake builds. Brendan, you should use build-webkit --gtkcmake until we switch --gtk to CMake :)
Created attachment 226170 [details] Patch I updated the changelogs and tested it on my machine, is there anything else we need here? (No idea why the style check is complaining about a random that I didn't change)
Attachment 226170 [details] did not pass style-queue: ERROR: Source/WebCore/PlatformGTK.cmake:380: There should be exactly one empty line instead of 0 between "Modules/geolocation/Geolocation.idl" and "Modules/mediasource/VideoPlaybackQuality.idl". [list/emptyline] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin, can you please review this one? I did a first pass, it was rolled-out, I fixed the CMake chunks for Brendan, so now I can't really review it again :)
Comment on attachment 226170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226170&action=review The CMake bits look good to me! > Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:184 > + if (getTag(tags.get(), GST_TAG_LANGUAGE_CODE, language)) { I'd kinda like to see this move to a function called getTagDescription. > Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:185 > + language = gst_tag_get_language_code_iso_639_1(language.utf8().data()); What kind of string is this? If it's UTF-8, you should use fromUTF8.
(In reply to comment #26) > > Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:184 > > + if (getTag(tags.get(), GST_TAG_LANGUAGE_CODE, language)) { > > I'd kinda like to see this move to a function called getTagDescription. Just rename getTag() to getTagDescription()? I think if we want a longer name, getTagContent() might be better. When I read "tag description" I think "human readable description of a tag", not the content. > > Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:185 > > + language = gst_tag_get_language_code_iso_639_1(language.utf8().data()); > > What kind of string is this? If it's UTF-8, you should use fromUTF8. They're ASCII. Should I use fromUTF8() for that?
Comment on attachment 226170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226170&action=review >> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:184 >> + if (getTag(tags.get(), GST_TAG_LANGUAGE_CODE, language)) { > > I'd kinda like to see this move to a function called getTagDescription. Yeah. I think you should wrap the process of getTag + gst_tag_get_language_code_iso_639_1 into a helper. Then you could do something like: if (getLanguageCode(..., language) && language != m_language) { m_language = language; client->languageChanged(m_owner, m_language); }
(In reply to comment #27) > > What kind of string is this? If it's UTF-8, you should use fromUTF8. > > They're ASCII. Should I use fromUTF8() for that? I think that fromUTF8 isn't necessary then.
Created attachment 226966 [details] Patch Moved the language code checking to getLanguageCode(). I realized while doing this that we should change m_label and m_language to AtomicStrings, but I assume that should be done seperately.
Comment on attachment 226966 [details] Patch Clearing flags on attachment: 226966 Committed r165763: <http://trac.webkit.org/changeset/165763>
(In reply to comment #31) > (From update of attachment 226966 [details]) > Clearing flags on attachment: 226966 > > Committed r165763: <http://trac.webkit.org/changeset/165763> It broke the WinCairo build: 1>WebCore.lib(TrackPrivateBaseGStreamer.obj) : error LNK2001: unresolved external symbol _gst_tag_get_language_code_iso_639_1 1>C:\Projects\BuildSlave\win-cairo-release\build\WebKitBuild\Release_WinCairo\bin32\WebKit.dll : fatal error LNK1120: 1 unresolved externals
(In reply to comment #33) > It broke the WinCairo build: > 1>WebCore.lib(TrackPrivateBaseGStreamer.obj) : error LNK2001: unresolved external symbol _gst_tag_get_language_code_iso_639_1 > 1>C:\Projects\BuildSlave\win-cairo-release\build\WebKitBuild\Release_WinCairo\bin32\WebKit.dll : fatal error LNK1120: 1 unresolved externals :( Why didn't EWS pick this up? I didn't realize the Windows build used GStreamer..
Reopening to attach new patch.
Created attachment 226975 [details] Patch Would this fix WinCairo? I don't have an easy way to test on Windows (I had assumed EWS would do that..).
(In reply to comment #36) > Created an attachment (id=226975) [details] > Patch > > Would this fix WinCairo? I don't have an easy way to test on Windows (I had assumed EWS would do that..). EWS says the patch won't apply, but I don't see any changes to that file in git or svn.. :\
Comment on attachment 226975 [details] Patch Someone else fixed WinCairo in r165763.