New issue
Advanced search Search tips

Issue 897767 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Pragmatically get mime types for a media test file

Project Member Reported by xhw...@chromium.org, Oct 22

Issue description

Currently the mime types for a media test file are hardcoded in each test, e.g.
- pipeline_integration_test.cc
- encrypted_media_browsertest.cc in chrome/ and content/
- media_source_browsertest.cc
This ends up with a lot of duplicate code and unnecessary complexity.

We should provide a helper function in media/test, such that for each test file, we can retrieve it's mime type, e.g. GetTestFileMimeType("bear-320x240-audio-only.webm") would return "audio/webm; codecs=\"vorbis\"". Then for each individual test, we don't need to specify the mime type at all, because the test function can call GetTestFileMimeType() to retrieve the mimetype.
 
This would also be a great thing to log to media internals for active playbacks.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/293e1434032be59bef17f7329a34718d9a9644b1

commit 293e1434032be59bef17f7329a34718d9a9644b1
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Mon Oct 22 21:39:01 2018

media: Misc cleanups for mime util GetStringToCodecMap()

- Define StringToCodecMap to avoid typing the long name multiple times.
- base::NoDestructor supports emplace; no need to create a map first.
- KEEP_FIRST_OF_DUPES is the default behavior; no need to specify.
- Remove #if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING), because:
  * It's causing clang format to misbehave.
  * We should keep the mapping simple and should not restrict feature
    here, especially since MimeUtil::Codec::EAC3 is defined
    unconditionally.
- Reformatted by "git cl format"

Bug:  897767 
Test: No functionality change.
Change-Id: Iac8d8b8ae5a9a1acbab89105c121f1d80badd536
Reviewed-on: https://chromium-review.googlesource.com/c/1294598
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601734}
[modify] https://crrev.com/293e1434032be59bef17f7329a34718d9a9644b1/media/base/mime_util_internal.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5526e3a499603c4c7b673443be68580fb3f2bd7c

commit 5526e3a499603c4c7b673443be68580fb3f2bd7c
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Tue Oct 23 18:57:24 2018

media: Add GetMimeTypeForFile() for test

This removes the duplicate code to specify mime type for a given test
file in multiple media tests.

In this CL chrome/content encrypted_media_browsertest.cc are updated to
use the new function.

In the next CL, media_source_browsertest.cc and
pipeline_integration_test.cc will be updated as well.

Bug:  897767 
Test: No functionality change.
Change-Id: I2e408d125fcc339352ad7c18a56b951f179f736c
Reviewed-on: https://chromium-review.googlesource.com/c/1295151
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602036}
[modify] https://crrev.com/5526e3a499603c4c7b673443be68580fb3f2bd7c/chrome/browser/media/encrypted_media_browsertest.cc
[modify] https://crrev.com/5526e3a499603c4c7b673443be68580fb3f2bd7c/content/browser/media/encrypted_media_browsertest.cc
[modify] https://crrev.com/5526e3a499603c4c7b673443be68580fb3f2bd7c/media/base/test_data_util.cc
[modify] https://crrev.com/5526e3a499603c4c7b673443be68580fb3f2bd7c/media/base/test_data_util.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09a09494971fba3dd8ec7573abc5d0c872136303

commit 09a09494971fba3dd8ec7573abc5d0c872136303
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Tue Oct 23 22:16:23 2018

media: Use GetMimeTypeForFile() in MediaSourceTest

This CL converts MediaSourceTest to use GetMimeTypeForFile() to get the
mime type for test files to remove duplicate code.

Bug:  897767 
Test: No functionality change
Change-Id: I5953cf8ae4e466d6fbb9c5d0ea021077a6f27f7f
Reviewed-on: https://chromium-review.googlesource.com/c/1296998
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602131}
[modify] https://crrev.com/09a09494971fba3dd8ec7573abc5d0c872136303/content/browser/media/media_source_browsertest.cc
[modify] https://crrev.com/09a09494971fba3dd8ec7573abc5d0c872136303/media/base/test_data_util.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21b87e697febf5a2688da3f17462997078f3e73a

commit 21b87e697febf5a2688da3f17462997078f3e73a
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Thu Oct 25 02:08:02 2018

media: Use GetMimeTypeForFile() in PipelineIntegrationTest

This CL converts PipelineIntegrationTest to use GetMimeTypeForFile()
to get the mime type for test files to remove duplicate code.

Bug:  897767 
Test: No functionality change
Change-Id: Id1520485b2bf8d46beb817013c2c40c22171ea8c
Reviewed-on: https://chromium-review.googlesource.com/c/1298240
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602579}
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/base/test_data_util.cc
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/remoting/integration_test.cc
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/test/data/README.md
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/test/mock_media_source.cc
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/test/mock_media_source.h
[modify] https://crrev.com/21b87e697febf5a2688da3f17462997078f3e73a/media/test/pipeline_integration_test.cc

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90a2c37bedefc9b02b7373e296387927da616cbd

commit 90a2c37bedefc9b02b7373e296387927da616cbd
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Thu Oct 25 18:48:44 2018

media: Drop "Only" suffix from mime type constants

Given we already specifies FooAudioBarVideo, "Only" seems redundant.

For example: kMp4Avc1VideoOnly -> kMp4Avc1Video

TBR=chcunningham@chromium.org

Bug:  897767 
Change-Id: I05d8ea2699e5d10a52f6e4978ee1cef7f153f60f
Reviewed-on: https://chromium-review.googlesource.com/c/1298509
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602813}
[modify] https://crrev.com/90a2c37bedefc9b02b7373e296387927da616cbd/media/base/test_data_util.cc

Sign in to add a comment