Bug 124514 – [GStreamer] human readable language code for tracks

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

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

[Brendan Long]

Which video were you looking at that had weird language codes?

[Philippe Normand]

(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

[Brendan Long]

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. :\

[Brendan Long]

Created attachment 217304 [details]
Patch

[Brendan Long]

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?

[EFL EWS Bot]

Comment on attachment 217304 [details]
Patch

Attachment 217304 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/27138158

[EFL EWS Bot]

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

[Philippe Normand]

(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.

[Philippe Normand]

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

[Brendan Long]

Created attachment 225942 [details]
Patch

[Brendan Long]

I also simplified the if (client) check to only be done once..

[WebKit Commit Bot]

Comment on attachment 225942 [details]
Patch

Clearing flags on attachment: 225942

Committed r165175: <http://trac.webkit.org/changeset/165175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129788

[Zan Dobersek]

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.

[Brendan Long]

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.

[Brendan Long]

(In reply to comment #16)
> I'm nto test this, 

"I'm not able to test this"

[Brendan Long]

Hm not sure why CMake doesn't like this. I'll spend more time trying to get the CMake build to work for me.

[Philippe Normand]

I'm going to check it on the GTK CMake build

[Philippe Normand]

Created attachment 226098 [details]
Patch

[WebKit Commit Bot]

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.

[Philippe Normand]

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 :)

[Brendan Long]

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)

[WebKit Commit Bot]

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.

[Philippe Normand]

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 :)

[Martin Robinson]

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.

[Brendan Long]

(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?

[Martin Robinson]

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);
}

[Martin Robinson]

(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.

[Brendan Long]

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.

[WebKit Commit Bot]

Comment on attachment 226966 [details]
Patch

Clearing flags on attachment: 226966

Committed r165763: <http://trac.webkit.org/changeset/165763>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Brendan Long]

(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..

[Brendan Long]

Reopening to attach new patch.

[Brendan Long]

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..).

[Brendan Long]

(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.. :\

[Brendan Long]

Comment on attachment 226975 [details]
Patch

Someone else fixed WinCairo in r165763.