Add unittests for media mojo clients and services. |
||||||||||||
Issue descriptionMedia mojo clients and services are supposed to be a simple transport layer but it could get complicated in many cases, e.g.: - The client could be constructed on one thread but then bound to a different thread. - Connection error could happen at any time. We should make sure our implementation is still compatible with the interface on such errors. - Lifetime of services. If a service uses StrongBinding, it'll be destructed on connection error/lost. We should make sure this is handled correctly (e.g. nothing tries to access the destroyed object). To best way to catch all of these is to add unittests. As we have more and more use of these classes, we should have heavy tests covering them. ⛆ |
|
|
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03fa1fc9ad9bc265dcebb93b0e8d5a47f764b8a6 commit 03fa1fc9ad9bc265dcebb93b0e8d5a47f764b8a6 Author: xhwang <xhwang@chromium.org> Date: Fri Jun 10 00:19:31 2016 media: Fix mojo DecoderBuffer type converter When side_data is empty, we should not called front(), otherwise it's undefined behavior: https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/array.h?rcl=0&l=97 The crash is like this: #2 0x00007ffff442dfe5 in __gnu_debug::_Error_formatter::_M_error (this=0x7fffffffc058) at ../../../../../src/libstdc++-v3/src/c++11/debug.cc:781 #3 0x0000000000550062 in std::__debug::vector<unsigned char, std::allocator<unsigned char> >::front (this=0x362504b6f3a8) at ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../in clude/c++/4.6/debug/vector:329 #4 0x000000000054d905 in mojo::Array<unsigned char>::front (this=0x362504b6f3a8) at ../../mojo/public/cpp/bindings/array.h:99 #5 0x000000000054b9b4 in mojo::TypeConverter<scoped_refptr<media::DecoderBuffer>, mojo::StructPtr<m edia::mojom::DecoderBuffer> >::Convert (input=...) at ../../media/mojo/common/media_type_converters.cc:476 I'll enable media_mojo_unittests on bots shortly so that we can catch regressions. BUG= 617204 TEST=fixes a unittest Review-Url: https://codereview.chromium.org/2050323002 Cr-Commit-Position: refs/heads/master@{#399054} [modify] https://crrev.com/03fa1fc9ad9bc265dcebb93b0e8d5a47f764b8a6/media/mojo/common/media_type_converters.cc
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/72421f05b895615110d7fcc43b56e31942f34c54 commit 72421f05b895615110d7fcc43b56e31942f34c54 Author: xhwang <xhwang@chromium.org> Date: Wed Jun 15 17:46:11 2016 media: Enable media_mojo_unittests on bots Also fix some BUILD.gn files. BUG= 617204 Review-Url: https://codereview.chromium.org/2053003002 Cr-Commit-Position: refs/heads/master@{#399947} [modify] https://crrev.com/72421f05b895615110d7fcc43b56e31942f34c54/BUILD.gn [modify] https://crrev.com/72421f05b895615110d7fcc43b56e31942f34c54/media/mojo/BUILD.gn [modify] https://crrev.com/72421f05b895615110d7fcc43b56e31942f34c54/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/72421f05b895615110d7fcc43b56e31942f34c54/testing/buildbot/gn_isolate_map.pyl
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/713cd6ebbed2081585792041aaf6cea5c704f699 commit 713cd6ebbed2081585792041aaf6cea5c704f699 Author: hcarmona <hcarmona@chromium.org> Date: Wed Jun 15 18:53:40 2016 Revert of media: Enable media_mojo_unittests on bots (patchset #5 id:80001 of https://codereview.chromium.org/2053003002/ ) Reason for revert: Fails to compile on https://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder%20%28dbg%29 Original issue's description: > media: Enable media_mojo_unittests on bots > > Also fix some BUILD.gn files. > > BUG= 617204 > > Committed: https://crrev.com/72421f05b895615110d7fcc43b56e31942f34c54 > Cr-Commit-Position: refs/heads/master@{#399947} TBR=jam@chromium.org,agable@chromium.org,thakis@chromium.org,jam@google.com,xhwang@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 617204 Review-Url: https://codereview.chromium.org/2071623002 Cr-Commit-Position: refs/heads/master@{#399978} [modify] https://crrev.com/713cd6ebbed2081585792041aaf6cea5c704f699/BUILD.gn [modify] https://crrev.com/713cd6ebbed2081585792041aaf6cea5c704f699/media/mojo/BUILD.gn [modify] https://crrev.com/713cd6ebbed2081585792041aaf6cea5c704f699/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/713cd6ebbed2081585792041aaf6cea5c704f699/testing/buildbot/gn_isolate_map.pyl
,
Jun 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f69024d1ddb7ee6b001203b13a019b6fc9dc7746 commit f69024d1ddb7ee6b001203b13a019b6fc9dc7746 Author: xhwang <xhwang@chromium.org> Date: Thu Jun 16 17:52:29 2016 (reland) media: Enable media_mojo_unittests on bots This reverts commit 713cd6ebbed2081585792041aaf6cea5c704f699 and relands 72421f05b895615110d7fcc43b56e31942f34c54 with fix. The test is only enabled on "ClangToTLinux tester". Original CL description: Also fix some BUILD.gn files. TBR=jam@chromium.org,thakis@chromium.org BUG= 617204 Review-Url: https://codereview.chromium.org/2069043003 Cr-Commit-Position: refs/heads/master@{#400197} [modify] https://crrev.com/f69024d1ddb7ee6b001203b13a019b6fc9dc7746/BUILD.gn [modify] https://crrev.com/f69024d1ddb7ee6b001203b13a019b6fc9dc7746/media/mojo/BUILD.gn [modify] https://crrev.com/f69024d1ddb7ee6b001203b13a019b6fc9dc7746/media/mojo/services/mojo_cdm_allocator_unittest.cc [modify] https://crrev.com/f69024d1ddb7ee6b001203b13a019b6fc9dc7746/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/f69024d1ddb7ee6b001203b13a019b6fc9dc7746/testing/buildbot/gn_isolate_map.pyl
,
Jun 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58cf81460b741ef317f54de1756e5cd855034bfe commit 58cf81460b741ef317f54de1756e5cd855034bfe Author: xhwang <xhwang@chromium.org> Date: Sat Jun 18 06:33:03 2016 media: Add unittest for MojoRenderer{Impl|Service} This setup the following chain to test both MojoRendererImpl and MojoRendererService: MojoRendererTest -> MojoRendererImpl -> (mojom::Renderer) -> MojoRendererService -> MockRenderer This CL also fixes some real issues in MojoRendererImpl: - After connection error, we may have outstanding callbacks not fired - If we call MojoRendererImpl after error, callbacks should still be fired. BUG= 617204 TEST=This adds new unittests. Review-Url: https://codereview.chromium.org/2070753002 Cr-Commit-Position: refs/heads/master@{#400581} [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/base/cdm_context.cc [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/base/cdm_context.h [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/base/mock_filters.cc [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/base/mock_filters.h [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/BUILD.gn [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/clients/BUILD.gn [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/clients/mojo_renderer_impl.cc [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/clients/mojo_renderer_impl.h [add] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/clients/mojo_renderer_unittest.cc [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/services/mojo_cdm_service.h [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/services/mojo_cdm_service_context.h [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/mojo/services/mojo_renderer_service.h [modify] https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe/media/renderers/renderer_impl_unittest.cc
,
Jun 23 2016
,
Jun 23 2016
,
Jun 23 2016
,
Jun 24 2016
Assign to jrummell to enable media_mojo_unitests on GN bots on the main waterfall. Thanks! Background: Today, media_mojo_unitests is only enabled on the ClangToTLinux%20tester bot on fyi waterfall: - CL that enables it: https://codereview.chromium.org/2053003002 - Bot: https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux%20tester - An example run: https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux%20tester/builds/1815/steps/media_mojo_unittests%20on%20Ubuntu-12.04/logs/stdio According to the codereview comments (https://codereview.chromium.org/2053003002#msg17), "if they work fine, add them to main waterfall bots quickly (say, after a few days of the tests being green)". So we should enable these tests on all GN bots (especially Windows ones if they exist, since Windows is special) on the main waterfall. You can refer to https://codereview.chromium.org/2087683002/ to see how to enable tests on the main waterfall (look at those json files). The main difference here is that media_mojo_unitests only works on GN bots, so we have to figure out whether a bot is running GN or GYP. I don't have an easy way to do this other than manually check the "generate_build_files" step on each bot and see whether it uses GYP_DEFINES, or gn args. You may ask infrastructure people about whether there's an easy way to do this. Due to the "after a few days" comment, mark this issue as M-53.
,
Jul 7 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 26 2016
,
Sep 15 2016
I am adding more tests to media_mojo_unitests, so it's more important that we enable this test on bots on the main waterfall. Assign back to myself to finish this work.
,
Sep 15 2016
FWIW, alokp@ has enabled media_mojo_unitests on the chromium linux bot (chromium.linux.json): https://codereview.chromium.org/2324483002
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/55b05c7a4c6722911ee2c715ed3a519da8d12315 commit 55b05c7a4c6722911ee2c715ed3a519da8d12315 Author: xhwang <xhwang@chromium.org> Date: Fri Sep 16 19:12:56 2016 media: Enable media_mojo_unittests on more bots BUG= 617204 TEST=This enables existing test on more bots Review-Url: https://codereview.chromium.org/2338023005 Cr-Commit-Position: refs/heads/master@{#419239} [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.chromiumos.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.full.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.mac.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.memory.full.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.memory.fyi.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.memory.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium.win.json [modify] https://crrev.com/55b05c7a4c6722911ee2c715ed3a519da8d12315/testing/buildbot/chromium_memory_trybot.json
,
Sep 16 2016
I'll close this bug now and individual tests will be tracked in the blocked-on bugs separately.
,
Sep 16 2016
|
|||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by bugdroid1@chromium.org
, Jun 10 2016