Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using findMediaFile(...) in LayoutTests #8672

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 13, 2017

Historically, findMediaFile() helped choose the right extension based
on what the browser could play. Chromium cannot play some proprietary
formats (h264, aac, etc), so this check would choose the .ogv format
in that case.

But things have gotten crufty.

  1. A long time ago we started running layout tests on builds that do
    include proprietary codecs
  2. But we lie about that to the tests via
    media::RemoveProprietaryMediaTypesAndCodecsForTests()
    This makes debugging tests a pain because manual execution doesn't
    call this method and its easy to forget that the test wrapper does.
  3. Lots of tests never bothered to call findMediaFile anway and just
    hardcoded the use of an mp4 with proprietary codecs, which worked
    because the method in Apple's submission #2 above only changes canPlayType responses,
    it doesn't remove the actual support for proprietary codecs.
  4. findMediaFile is about to be busted anyway because it only queries the
    file mime type without supplying codec info. eg
    canPlayType("video/mp4") == "maybe" -> lets use the mp4!
    and even chromium will now "maybe" for this now that we no longer
    consider mp4 proprietary (though codecs like h264 still are!)

So this patch does the following:

  1. Deletes findMediaFile() and instead hardcode use of the ogg file. This
    maintains the existing behavior and avoids a massive rebaseline.
  2. Delete media::RemoveProprietaryMediaTypesAndCodecsForTests(). Tests
    and manual runs now behave the same.
  3. Delete ancient media-can-play-* LayoutTests that just call canPlayType
    with various codecs and are duplicated by content and chrome browser
    tests (which is a better place for these checks)
  4. Updates mediasource-config-change-mp4-* expectations to expect that
    they should run and pass on all platforms (previously just android)

There are still many tests that hard code the use of mp4 files. If we
later desire to see LayoutTests run without proprietary codecs, someone
can transition those tests. No one seems to mind at the moment.

It is also not a goal to make LayoutTests try all the supported codecs.
That is covered by unit/integration tests.

Due to #2 above, a handful of tests in external/wpt/media-source now fail
because they use mp4. Fixing these failures is tracked in Issue 794338.

Change-Id: Ie357ae075c880b78d5ee9e95c1b7cc69d9d8a328
BUG: 327115,746579,787575,568704,794338
Reviewed-on: https://chromium-review.googlesource.com/807604
Reviewed-by: Dale Curtis [email protected]
Reviewed-by: Peter Beverloo [email protected]
Commit-Queue: Chrome Cunningham [email protected]
Cr-Commit-Position: refs/heads/master@{#523821}


This change is Reviewable

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@ghost
Copy link

ghost commented Dec 13, 2017

Build PASSED

Started: 2017-12-13 18:31:59
Finished: 2017-12-13 18:36:05

View more information about this build on:

Historically, findMediaFile() helped choose the right extension based
on what the browser could play. Chromium cannot play some proprietary
formats (h264, aac, etc), so this check would choose the .ogv format
in that case.

But things have gotten crufty.
1) A long time ago we started running layout tests on builds that *do*
   include proprietary codecs
2) But we lie about that to the tests via
     media::RemoveProprietaryMediaTypesAndCodecsForTests()
   This makes debugging tests a pain because manual execution doesn't
   call this method and its easy to forget that the test wrapper does.
3) Lots of tests never bothered to call findMediaFile anway and just
   hardcoded the use of an mp4 with proprietary codecs, which worked
   because the method in #2 above only changes canPlayType responses,
   it doesn't remove the actual support for proprietary codecs.
4) findMediaFile is about to be busted anyway because it only queries the
   file mime type without supplying codec info. eg
      canPlayType("video/mp4") == "maybe" -> lets use the mp4!
   and even chromium will now "maybe" for this now that we no longer
   consider mp4 proprietary (though codecs like h264 still are!)

So this patch does the following:

1) Deletes findMediaFile() and instead hardcode use of the ogg file. This
   maintains the existing behavior and avoids a massive rebaseline.
2) Delete media::RemoveProprietaryMediaTypesAndCodecsForTests(). Tests
   and manual runs now behave the same.
3) Delete ancient media-can-play-* LayoutTests that just call canPlayType
   with various codecs and are duplicated by content and chrome browser
   tests (which is a better place for these checks)
4) Updates mediasource-config-change-mp4-* expectations to expect that
   they should run and pass on *all* platforms (previously just android)

There are still many tests that hard code the use of mp4 files. If we
later desire to see LayoutTests run without proprietary codecs, someone
can transition those tests. No one seems to mind at the moment.

It is also not a goal to make LayoutTests try all the supported codecs.
That is covered by unit/integration tests.

Due to #2 above, a handful of tests in external/wpt/media-source now fail
because they use mp4. Fixing these failures is tracked in Issue 794338.

Change-Id: Ie357ae075c880b78d5ee9e95c1b7cc69d9d8a328
BUG: 327115,746579,787575,568704,794338
Reviewed-on: https://chromium-review.googlesource.com/807604
Reviewed-by: Dale Curtis <[email protected]>
Reviewed-by: Peter Beverloo <[email protected]>
Commit-Queue: Chrome Cunningham <[email protected]>
Cr-Commit-Position: refs/heads/master@{#523821}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants