Bug 79519 – [BlackBerry] Add support for FLAC audio and OGG/Vorbis audio

• Component: Media
• Status: CLOSED FIXED

[Max Feil]

New mime types (Internet Media Types) and file extensions need to be added for HTML5 audio to support FLAC and OGG/Vorbis. There are also changes required in the platform and libwebview repositories but these will be reviewed separately.

[Max Feil]

Created attachment 128808 [details]
Proposed patch for review

[WebKit Review Bot]

Comment on attachment 128808 [details]
Proposed patch for review

Attachment 128808 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11628196

New failing tests:
media/media-can-play-flac-audio.html

[Max Feil]

Ok, obviously I need to add the new layout test to the Skipped file for a number of upstream ports. But how do I figure out which ones? All of them? (except Blackberry)

[Antonio Gomes]

Comment on attachment 128808 [details]
Proposed patch for review

What you could do as well, is add the test in LayoutTests/platform/blackberry/media/media-can-play-flac-audio.html . So it will be a platform specific test. I think we need to upstream our tests machinery first though right?

[Max Feil]

Created attachment 129090 [details]
Updated patch with new flac layout test skipped where necessary

[Antonio Gomes]

Comment on attachment 129090 [details]
Updated patch with new flac layout test skipped where necessary

So Max, any port wont skip it other than BlackBerry port?

[Max Feil]

I added the new layout test to all the "Skipped" files where it made sense. If the Skipped file did not look like it was being maintained, for example if it was very short or blank, I didn't add the test. Here are the Skipped files I am not touching:

LayoutTests/platform/gtk-wk2/Skipped
LayoutTests/platform/mac-wk2/Skipped
LayoutTests/platform/qt-4.8/Skipped
LayoutTests/platform/qt-5.0/Skipped
LayoutTests/platform/qt-5.0-wk1/Skipped
LayoutTests/platform/qt-5.0-wk2/Skipped
LayoutTests/platform/qt-arm/Skipped
LayoutTests/platform/qt-linux/Skipped
LayoutTests/platform/win-wk2/Skipped
LayoutTests/platform/win-xp/Skipped
LayoutTests/platform/wk2/Skipped

I was assuming the WebKit Review Bot would catch any failing tests, like it did in Comment #2.

[Antonio Gomes]

> LayoutTests/platform/qt-4.8/Skipped
> LayoutTests/platform/qt-5.0/Skipped
> LayoutTests/platform/qt-5.0-wk1/Skipped
> LayoutTests/platform/qt-5.0-wk2/Skipped
> LayoutTests/platform/qt-arm/Skipped
> LayoutTests/platform/qt-linux/Skipped

If you want to skip it to Qt generally speaking, you do not need to all it to each different Skip file, but to LayoutTests/platform/qt/Skipped

> LayoutTests/platform/win-wk2/Skipped
> LayoutTests/platform/wk2/Skipped
> LayoutTests/platform/gtk-wk2/Skipped
> LayoutTests/platform/mac-wk2/Skipped

I believe this one skips the test for all wk2 ports.


Any my previous question come again: is any ports supposed to run this test other than BlackBerry?

[Max Feil]

Hi Antonio. My intention is to make the test available to any upstream ports that wish to use it, but not introduce errors in the upstream builds. To me it just makes sense to put the new test in the same place as the other media-can-play tests such as ogg.

Also, maybe you misunderstood the list in my previous comment. Those were the files that I didn't touch. The ones I did modify are:

LayoutTests/platform/chromium/test_expectations.txt
LayoutTests/platform/efl/Skipped
LayoutTests/platform/gtk/Skipped
LayoutTests/platform/mac-leopard/Skipped
LayoutTests/platform/mac-lion/Skipped
LayoutTests/platform/mac-snowleopard/Skipped
LayoutTests/platform/mac/Skipped
LayoutTests/platform/qt-mac/Skipped
LayoutTests/platform/qt-win/Skipped
LayoutTests/platform/qt/Skipped
LayoutTests/platform/win/Skipped
LayoutTests/platform/wincairo/Skipped

Do you see any that are redundant? The hierarchy of the files is not obvious to me. Are you saying that tests skipped in LayoutTests/platform/mac/Skipped do not need to be separately specified in, for example, LayoutTests/platform/mac-snowleopard/Skipped? If that is the case, there are other redundant tests listed as well, specifically media/media-can-play-ogg.html is listed in both those files.

[Antonio Gomes]

> Are you saying that tests skipped in LayoutTests/platform/mac/Skipped do not need to be separately specified in, for example, LayoutTests/platform/mac-snowleopard/Skipped? If that is the case, there are other redundant tests listed as well, specifically media/media-can-play-ogg.html is listed in both those files.

That is what I am saying, yes :-)

[Max Feil]

Created attachment 129337 [details]
Updated patch with redundant "Skipped" file changes removed

[WebKit Review Bot]

Attachment 129337 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:489:  Path does not exist. media/media-can-play-flac-audio.html  [test/expectations] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Max Feil]

(In reply to comment #12)
> Attachment 129337 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> LayoutTests/platform/chromium/test_expectations.txt:489:  Path does not exist. media/media-can-play-flac-audio.html  [test/expectations] [5]
> Total errors found: 1 in 12 files

This seems really dumb. Can't add a test that fails because it's not being skipped. Can't add it to be skipped because the test doesn't exist yet. How do we avoid chicken-egg cases like this? All this automated checking seems like it makes getting any work done at all difficult...

Or am I missing something here?

[Adam Barth]

> This seems really dumb. Can't add a test that fails because it's not being skipped. Can't add it to be skipped because the test doesn't exist yet. How do we avoid chicken-egg cases like this? All this automated checking seems like it makes getting any work done at all difficult...

Traditionally folks add both the test and the instruction to skip the test at the same time.

[Max Feil]

Created attachment 129383 [details]
Trying the last patch again - was missing the new test

Thanks Adam. In my GIT fiddling to remove some files I forgot to add back in the new files.

[Max Feil]

Created attachment 129384 [details]
Trying the last patch again - was missing the new test

[Antonio Gomes]

Comment on attachment 129384 [details]
Trying the last patch again - was missing the new test

And my question comes again: what upstream port runs this test you are adding?

[Max Feil]

(In reply to comment #17)
> (From update of attachment 129384 [details])
> And my question comes again: what upstream port runs this test you are adding?

My intention is to make the test available to upstream ports and let them turn it on as desired. So Blackberry is the only port currently intended to run the test, even though it is potentially useful for other ports.

[Antonio Gomes]

please keep one eye at the bot, Max.

Thanks

[WebKit Review Bot]

Comment on attachment 129384 [details]
Trying the last patch again - was missing the new test

Clearing flags on attachment: 129384

Committed r109238: <http://trac.webkit.org/changeset/109238>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

Closing bug for patch that landed a long time ago.