Bug 124514 - [GStreamer] human readable language code for tracks
Summary: [GStreamer] human readable language code for tracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brendan Long
URL:
Keywords:
Depends on: 129788
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-18 09:04 PST by Philippe Normand
Modified: 2014-03-18 15:59 PDT (History)
22 users (show)

See Also:


Attachments
Patch (2.79 KB, patch)
2013-11-19 09:26 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (2.59 KB, patch)
2014-03-05 19:43 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2014-03-06 12:26 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (8.22 KB, patch)
2014-03-07 01:40 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2014-03-07 14:54 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2014-03-17 14:27 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2014-03-17 16:05 PDT, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-11-18 09:04:47 PST
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
Comment 1 Brendan Long 2013-11-18 11:47:36 PST
Which video were you looking at that had weird language codes?
Comment 2 Philippe Normand 2013-11-18 13:30:09 PST
(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
Comment 3 Brendan Long 2013-11-19 09:26:02 PST
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. :\
Comment 4 Brendan Long 2013-11-19 09:26:37 PST
Created attachment 217304 [details]
Patch
Comment 5 Brendan Long 2013-11-19 09:27:23 PST
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 6 EFL EWS Bot 2013-11-19 09:44:40 PST
Comment on attachment 217304 [details]
Patch

Attachment 217304 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/27138158
Comment 7 EFL EWS Bot 2013-11-19 10:06:23 PST
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
Comment 8 Philippe Normand 2013-12-20 05:42:17 PST
(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 9 Philippe Normand 2014-03-05 00:30:15 PST
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
Comment 10 Brendan Long 2014-03-05 19:43:54 PST
Created attachment 225942 [details]
Patch
Comment 11 Brendan Long 2014-03-05 19:44:42 PST
I also simplified the if (client) check to only be done once..
Comment 12 WebKit Commit Bot 2014-03-05 23:57:10 PST
Comment on attachment 225942 [details]
Patch

Clearing flags on attachment: 225942

Committed r165175: <http://trac.webkit.org/changeset/165175>
Comment 13 WebKit Commit Bot 2014-03-05 23:57:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Commit Bot 2014-03-06 00:47:18 PST
Re-opened since this is blocked by bug 129788
Comment 15 Zan Dobersek 2014-03-06 00:50:44 PST
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.
Comment 16 Brendan Long 2014-03-06 12:26:20 PST
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.
Comment 17 Brendan Long 2014-03-06 12:28:05 PST
(In reply to comment #16)
> I'm nto test this, 

"I'm not able to test this"
Comment 18 Brendan Long 2014-03-06 16:22:56 PST
Hm not sure why CMake doesn't like this. I'll spend more time trying to get the CMake build to work for me.
Comment 19 Philippe Normand 2014-03-06 23:55:12 PST
I'm going to check it on the GTK CMake build
Comment 20 Philippe Normand 2014-03-07 01:40:48 PST
Created attachment 226098 [details]
Patch
Comment 21 WebKit Commit Bot 2014-03-07 01:42:28 PST
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 22 Philippe Normand 2014-03-07 01:44:20 PST
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 :)
Comment 23 Brendan Long 2014-03-07 14:54:16 PST
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)
Comment 24 WebKit Commit Bot 2014-03-07 14:56:30 PST
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.
Comment 25 Philippe Normand 2014-03-08 01:41:06 PST
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 26 Martin Robinson 2014-03-11 08:17:29 PDT
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.
Comment 27 Brendan Long 2014-03-14 14:51:34 PDT
(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 28 Martin Robinson 2014-03-14 15:00:11 PDT
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);
}
Comment 29 Martin Robinson 2014-03-14 15:01:27 PDT
(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.
Comment 30 Brendan Long 2014-03-17 14:27:07 PDT
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 31 WebKit Commit Bot 2014-03-17 15:13:30 PDT
Comment on attachment 226966 [details]
Patch

Clearing flags on attachment: 226966

Committed r165763: <http://trac.webkit.org/changeset/165763>
Comment 32 WebKit Commit Bot 2014-03-17 15:13:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Csaba Osztrogonác 2014-03-17 15:50:56 PDT
(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
Comment 34 Brendan Long 2014-03-17 15:59:19 PDT
(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..
Comment 35 Brendan Long 2014-03-17 16:04:55 PDT
Reopening to attach new patch.
Comment 36 Brendan Long 2014-03-17 16:05:06 PDT
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..).
Comment 37 Brendan Long 2014-03-17 16:12:04 PDT
(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 38 Brendan Long 2014-03-18 15:57:27 PDT
Comment on attachment 226975 [details]
Patch

Someone else fixed WinCairo in r165763.