From ba3e254483c18238b25445b48d9d3592d40689a3 Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Mon, 17 Jul 2017 13:59:21 -0700 Subject: [PATCH] Implement out-of-band metadata images. This saves a lot of HIDL bandwidth, by not including raw image data in metadata vector. Bug: b/63702941 Test: VTS Change-Id: I73d5218095e4af34c58da8dcfc520abd4cb46c26 --- broadcastradio/1.0/types.hal | 11 +- .../VtsHalBroadcastradioV1_0TargetTest.cpp | 49 +++++- broadcastradio/1.1/IBroadcastRadio.hal | 19 +++ broadcastradio/1.1/default/BroadcastRadio.cpp | 15 ++ broadcastradio/1.1/default/BroadcastRadio.h | 1 + broadcastradio/1.1/default/Tuner.cpp | 5 +- broadcastradio/1.1/default/VirtualProgram.cpp | 21 +++ broadcastradio/1.1/default/VirtualProgram.h | 3 + broadcastradio/1.1/default/VirtualRadio.cpp | 4 +- broadcastradio/1.1/default/VirtualRadio.h | 2 +- broadcastradio/1.1/default/resources.h | 46 ++++++ .../VtsHalBroadcastradioV1_1TargetTest.cpp | 141 ++++++++++++++---- current.txt | 2 +- 13 files changed, 276 insertions(+), 43 deletions(-) create mode 100644 broadcastradio/1.1/default/resources.h diff --git a/broadcastradio/1.0/types.hal b/broadcastradio/1.0/types.hal index d165e32250..e9ac4b7d30 100644 --- a/broadcastradio/1.0/types.hal +++ b/broadcastradio/1.0/types.hal @@ -146,8 +146,11 @@ enum MetadataType : int32_t { /** String */ TEXT = 1, /** - * Raw binary data (icon or art) - This data must be transparent to the android framework */ + * Raw binary data (icon or art). + * + * The data should be a valid PNG, JPEG, GIF or BMP file. + * Invalid format must be handled gracefully as if the field was missing. + */ RAW = 2, /** clock data, see MetaDataClock */ CLOCK = 3, @@ -173,9 +176,9 @@ enum MetadataKey : int32_t { ALBUM = 7, /** Musical genre - string */ GENRE = 8, - /** Station icon - raw */ + /** Station icon - raw (int32_t for HAL 1.1) */ ICON = 9, - /** Album art - raw */ + /** Album art - raw (int32_t for HAL 1.1) */ ART = 10, /** Clock - MetaDataClock */ CLOCK = 11, diff --git a/broadcastradio/1.0/vts/functional/VtsHalBroadcastradioV1_0TargetTest.cpp b/broadcastradio/1.0/vts/functional/VtsHalBroadcastradioV1_0TargetTest.cpp index 7241ad4c37..fa0f030d09 100644 --- a/broadcastradio/1.0/vts/functional/VtsHalBroadcastradioV1_0TargetTest.cpp +++ b/broadcastradio/1.0/vts/functional/VtsHalBroadcastradioV1_0TargetTest.cpp @@ -46,7 +46,8 @@ using ::android::hardware::broadcastradio::V1_0::BandConfig; using ::android::hardware::broadcastradio::V1_0::Direction; using ::android::hardware::broadcastradio::V1_0::ProgramInfo; using ::android::hardware::broadcastradio::V1_0::MetaData; - +using ::android::hardware::broadcastradio::V1_0::MetadataKey; +using ::android::hardware::broadcastradio::V1_0::MetadataType; #define RETURN_IF_SKIPPED \ if (skipped) { \ @@ -648,6 +649,52 @@ TEST_P(BroadcastRadioHidlTest, TuneFailsOutOfBounds) { EXPECT_TRUE(waitForCallback(kTuneCallbacktimeoutNs)); } +/** + * Test proper image format in metadata. + * + * Verifies that: + * - all images in metadata are provided in-band (as a binary blob, not by id) + * + * This is a counter-test for OobImagesOnly from 1.1 VTS. + */ +TEST_P(BroadcastRadioHidlTest, IbImagesOnly) { + RETURN_IF_SKIPPED; + ASSERT_TRUE(openTuner()); + ASSERT_TRUE(checkAntenna()); + + bool firstScan = true; + uint32_t firstChannel, prevChannel; + while (true) { + mCallbackCalled = false; + auto hidlResult = mTuner->scan(Direction::UP, true); + ASSERT_TRUE(hidlResult.isOk()); + if (hidlResult == Result::TIMEOUT) { + ALOGI("Got timeout on scan operation"); + break; + } + ASSERT_EQ(Result::OK, hidlResult); + ASSERT_EQ(true, waitForCallback(kTuneCallbacktimeoutNs)); + + if (firstScan) { + firstScan = false; + firstChannel = mProgramInfoCallbackData.channel; + } else { + // scanned the whole band + if (mProgramInfoCallbackData.channel >= firstChannel && prevChannel <= firstChannel) { + break; + } + } + prevChannel = mProgramInfoCallbackData.channel; + + for (auto&& entry : mProgramInfoCallbackData.metadata) { + if (entry.key != MetadataKey::ICON && entry.key != MetadataKey::ART) continue; + EXPECT_EQ(MetadataType::RAW, entry.type); + EXPECT_EQ(0, entry.intValue); + EXPECT_GT(entry.rawValue.size(), 0u); + } + } +} + INSTANTIATE_TEST_CASE_P( BroadcastRadioHidlTestCases, BroadcastRadioHidlTest, diff --git a/broadcastradio/1.1/IBroadcastRadio.hal b/broadcastradio/1.1/IBroadcastRadio.hal index dd37d4942c..9bde361bdb 100644 --- a/broadcastradio/1.1/IBroadcastRadio.hal +++ b/broadcastradio/1.1/IBroadcastRadio.hal @@ -27,4 +27,23 @@ interface IBroadcastRadio extends @1.0::IBroadcastRadio { */ getProperties_1_1() generates (Properties properties); + /** + * Fetch image from radio module. + * + * This call is meant to make V1_0::MetaData lightweight - instead of + * passing image data blob in MetadataType.RAW field, only identifier is + * passed, so the client may cache images or even not fetch them. + * + * Identifier may be any arbitrary number - sequential, sha256 prefix, + * or any other unique value selected by the vendor. + * + * The data should be a valid PNG, JPEG, GIF or BMP file. + * Invalid format must be handled gracefully as if the image was missing. + * + * @param id Identifier of an image; + * value of 0 is reserved and should be treated as invalid image. + * @return image A binary blob with image data + * or zero-length vector if identifier doesn't exists. + */ + getImage(int32_t id) generates (vec image); }; diff --git a/broadcastradio/1.1/default/BroadcastRadio.cpp b/broadcastradio/1.1/default/BroadcastRadio.cpp index 297dcc1cee..ab3047d39a 100644 --- a/broadcastradio/1.1/default/BroadcastRadio.cpp +++ b/broadcastradio/1.1/default/BroadcastRadio.cpp @@ -20,6 +20,8 @@ #include +#include "resources.h" + namespace android { namespace hardware { namespace broadcastradio { @@ -155,6 +157,19 @@ Return BroadcastRadio::openTuner(const BandConfig& config, bool audio __un return Void(); } +Return BroadcastRadio::getImage(int32_t id, getImage_cb _hidl_cb) { + ALOGV("%s(%x)", __func__, id); + + if (id == resources::demoPngId) { + _hidl_cb(std::vector(resources::demoPng, std::end(resources::demoPng))); + return {}; + } + + ALOGI("Image %x doesn't exists", id); + _hidl_cb({}); + return Void(); +} + } // namespace implementation } // namespace V1_1 } // namespace broadcastradio diff --git a/broadcastradio/1.1/default/BroadcastRadio.h b/broadcastradio/1.1/default/BroadcastRadio.h index d98afa7acb..71e3be8aec 100644 --- a/broadcastradio/1.1/default/BroadcastRadio.h +++ b/broadcastradio/1.1/default/BroadcastRadio.h @@ -63,6 +63,7 @@ struct BroadcastRadio : public V1_1::IBroadcastRadio { Return openTuner(const V1_0::BandConfig& config, bool audio, const sp& callback, openTuner_cb _hidl_cb) override; + Return getImage(int32_t id, getImage_cb _hidl_cb); private: std::mutex mMut; diff --git a/broadcastradio/1.1/default/Tuner.cpp b/broadcastradio/1.1/default/Tuner.cpp index 271f6337ed..0a452088fd 100644 --- a/broadcastradio/1.1/default/Tuner.cpp +++ b/broadcastradio/1.1/default/Tuner.cpp @@ -52,7 +52,10 @@ const struct { Tuner::Tuner(const sp& callback) : mCallback(callback), mCallback1_1(ITunerCallback::castFrom(callback).withDefault(nullptr)), - mVirtualFm(make_fm_radio()) {} + mVirtualFm(make_fm_radio()) { + // TODO (b/36864090): inject this data in a more elegant way + setCompatibilityLevel(mCallback1_1 == nullptr ? 1 : 2); +} void Tuner::forceClose() { lock_guard lk(mMut); diff --git a/broadcastradio/1.1/default/VirtualProgram.cpp b/broadcastradio/1.1/default/VirtualProgram.cpp index babf0d8b60..1f3b69310b 100644 --- a/broadcastradio/1.1/default/VirtualProgram.cpp +++ b/broadcastradio/1.1/default/VirtualProgram.cpp @@ -17,6 +17,8 @@ #include +#include "resources.h" + namespace android { namespace hardware { namespace broadcastradio { @@ -27,6 +29,23 @@ using V1_0::MetaData; using V1_0::MetadataKey; using V1_0::MetadataType; +// TODO (b/36864090): inject this data in a more elegant way +static int gHalVersion = 2; // 1 = 1.0, 2 = 1.1 + +void setCompatibilityLevel(int halversion) { + gHalVersion = halversion; +} + +static MetaData createDemoBitmap(MetadataKey key) { + MetaData bmp = {MetadataType::INT, key, resources::demoPngId, {}, {}, {}}; + if (gHalVersion < 2) { + bmp.type = MetadataType::RAW; + bmp.intValue = 0; + bmp.rawValue = std::vector(resources::demoPng, std::end(resources::demoPng)); + } + return bmp; +} + VirtualProgram::operator ProgramInfo() const { ProgramInfo info11 = {}; auto& info10 = info11.base; @@ -42,6 +61,8 @@ VirtualProgram::operator ProgramInfo() const { {MetadataType::TEXT, MetadataKey::RDS_PS, {}, {}, programName, {}}, {MetadataType::TEXT, MetadataKey::TITLE, {}, {}, songTitle, {}}, {MetadataType::TEXT, MetadataKey::ARTIST, {}, {}, songArtist, {}}, + createDemoBitmap(MetadataKey::ICON), + createDemoBitmap(MetadataKey::ART), }); return info11; diff --git a/broadcastradio/1.1/default/VirtualProgram.h b/broadcastradio/1.1/default/VirtualProgram.h index a4fd72c32b..2ee21a7064 100644 --- a/broadcastradio/1.1/default/VirtualProgram.h +++ b/broadcastradio/1.1/default/VirtualProgram.h @@ -25,6 +25,9 @@ namespace broadcastradio { namespace V1_1 { namespace implementation { +// TODO (b/36864090): inject this data in a more elegant way +void setCompatibilityLevel(int halversion); + struct VirtualProgram { ProgramSelector selector; diff --git a/broadcastradio/1.1/default/VirtualRadio.cpp b/broadcastradio/1.1/default/VirtualRadio.cpp index acf37a40e7..89b2b0ae2d 100644 --- a/broadcastradio/1.1/default/VirtualRadio.cpp +++ b/broadcastradio/1.1/default/VirtualRadio.cpp @@ -32,7 +32,7 @@ using std::vector; using utils::make_selector; -vector gInitialFmPrograms{ +const vector gInitialFmPrograms{ {make_selector(Band::FM, 94900), "Wild 94.9", "Drake ft. Rihanna", "Too Good"}, {make_selector(Band::FM, 96500), "KOIT", "Celine Dion", "All By Myself"}, {make_selector(Band::FM, 97300), "Alice@97.3", "Drops of Jupiter", "Train"}, @@ -44,7 +44,7 @@ vector gInitialFmPrograms{ VirtualRadio::VirtualRadio(VirtualRadio&& o) : mPrograms(move(o.mPrograms)) {} -VirtualRadio::VirtualRadio(vector initialList) : mPrograms(initialList) {} +VirtualRadio::VirtualRadio(const vector initialList) : mPrograms(initialList) {} vector VirtualRadio::getProgramList() { lock_guard lk(mMut); diff --git a/broadcastradio/1.1/default/VirtualRadio.h b/broadcastradio/1.1/default/VirtualRadio.h index 23cb06c619..4cdc72ffc8 100644 --- a/broadcastradio/1.1/default/VirtualRadio.h +++ b/broadcastradio/1.1/default/VirtualRadio.h @@ -30,7 +30,7 @@ namespace implementation { class VirtualRadio { public: VirtualRadio(VirtualRadio&& o); - VirtualRadio(std::vector initialList); + VirtualRadio(const std::vector initialList); std::vector getProgramList(); bool getProgram(const ProgramSelector& selector, VirtualProgram& program); diff --git a/broadcastradio/1.1/default/resources.h b/broadcastradio/1.1/default/resources.h new file mode 100644 index 0000000000..b7e709f955 --- /dev/null +++ b/broadcastradio/1.1/default/resources.h @@ -0,0 +1,46 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef ANDROID_HARDWARE_BROADCASTRADIO_V1_1_RESOURCES_H +#define ANDROID_HARDWARE_BROADCASTRADIO_V1_1_RESOURCES_H + +namespace android { +namespace hardware { +namespace broadcastradio { +namespace V1_1 { +namespace implementation { +namespace resources { + +constexpr int32_t demoPngId = 123456; +constexpr uint8_t demoPng[] = { + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, + 0x52, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x40, 0x08, 0x02, 0x00, 0x00, 0x00, 0x25, + 0x0b, 0xe6, 0x89, 0x00, 0x00, 0x00, 0x5d, 0x49, 0x44, 0x41, 0x54, 0x68, 0xde, 0xed, 0xd9, + 0xc1, 0x09, 0x00, 0x30, 0x08, 0x04, 0xc1, 0x33, 0xfd, 0xf7, 0x6c, 0x6a, 0xc8, 0x23, 0x04, + 0xc9, 0x6c, 0x01, 0xc2, 0x20, 0xbe, 0x4c, 0x86, 0x57, 0x49, 0xba, 0xfb, 0xd6, 0xf4, 0xba, + 0x3e, 0x7f, 0x4d, 0xdf, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc0, 0x8f, 0x00, 0xbd, 0xce, 0x7f, + 0xc0, 0x11, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe8, 0xb8, 0x0d, 0x32, 0xd4, 0x0c, 0x77, 0xbd, + 0xfb, 0xc1, 0xce, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82}; + +} // namespace resources +} // namespace implementation +} // namespace V1_1 +} // namespace broadcastradio +} // namespace hardware +} // namespace android + +#endif // ANDROID_HARDWARE_BROADCASTRADIO_V1_1_RESOURCES_H diff --git a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp index 9e7c00bc91..d20452bce5 100644 --- a/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp +++ b/broadcastradio/1.1/vts/functional/VtsHalBroadcastradioV1_1TargetTest.cpp @@ -54,6 +54,8 @@ using broadcastradio::vts::CallBarrier; using V1_0::BandConfig; using V1_0::Class; using V1_0::MetaData; +using V1_0::MetadataKey; +using V1_0::MetadataType; static constexpr auto kConfigTimeout = 10s; static constexpr auto kConnectModuleTimeout = 1s; @@ -93,6 +95,7 @@ class BroadcastRadioHalTest : public ::testing::VtsHalHidlTargetTestBase, // any stations on the list, so don't pick AM blindly). bool openTuner(unsigned band); const BandConfig& getBand(unsigned idx); + bool getProgramList(std::function& list)> cb); Class radioClass; bool skipped = false; @@ -207,6 +210,47 @@ const BandConfig& BroadcastRadioHalTest::getBand(unsigned idx) { return band; } +bool BroadcastRadioHalTest::getProgramList( + std::function& list)> cb) { + ProgramListResult getListResult = ProgramListResult::NOT_INITIALIZED; + bool isListEmpty = true; + auto getListCb = [&](ProgramListResult result, const hidl_vec& list) { + ALOGD("getListCb(%s, ProgramInfo[%zu])", toString(result).c_str(), list.size()); + getListResult = result; + if (result != ProgramListResult::OK) return; + isListEmpty = (list.size() == 0); + if (!isListEmpty) cb(list); + }; + + // first try... + EXPECT_TIMEOUT_CALL(*mCallback, backgroundScanComplete, ProgramListResult::OK) + .Times(AnyNumber()); + auto hidlResult = mTuner->getProgramList("", getListCb); + EXPECT_TRUE(hidlResult.isOk()); + if (!hidlResult.isOk()) return false; + + if (getListResult == ProgramListResult::NOT_STARTED) { + auto result = mTuner->startBackgroundScan(); + EXPECT_EQ(ProgramListResult::OK, result); + getListResult = ProgramListResult::NOT_READY; // continue as in NOT_READY case + } + if (getListResult == ProgramListResult::NOT_READY) { + EXPECT_TIMEOUT_CALL_WAIT(*mCallback, backgroundScanComplete, kFullScanTimeout); + + // second (last) try... + hidlResult = mTuner->getProgramList("", getListCb); + EXPECT_TRUE(hidlResult.isOk()); + if (!hidlResult.isOk()) return false; + EXPECT_EQ(ProgramListResult::OK, getListResult); + } + + if (isListEmpty) { + printSkipped("Program list is empty."); + return false; + } + return true; +} + /** * Test IBroadcastRadio::openTuner() method called twice. * @@ -242,41 +286,11 @@ TEST_P(BroadcastRadioHalTest, TuneFromProgramList) { ASSERT_TRUE(openTuner(0)); ProgramInfo firstProgram; - bool isListEmpty; - ProgramListResult getListResult = ProgramListResult::NOT_INITIALIZED; - auto getListCb = [&](ProgramListResult result, const hidl_vec& list) { - ALOGD("getListCb(%s, ProgramInfo[%zu])", toString(result).c_str(), list.size()); - getListResult = result; - if (result != ProgramListResult::OK) return; - isListEmpty = (list.size() == 0); + auto getCb = [&](const hidl_vec& list) { // don't copy the whole list out, it might be heavy - if (!isListEmpty) firstProgram = list[0]; + firstProgram = list[0]; }; - - // first try... - EXPECT_TIMEOUT_CALL(*mCallback, backgroundScanComplete, ProgramListResult::OK) - .Times(AnyNumber()); - auto hidlResult = mTuner->getProgramList("", getListCb); - ASSERT_TRUE(hidlResult.isOk()); - - if (getListResult == ProgramListResult::NOT_STARTED) { - auto result = mTuner->startBackgroundScan(); - ASSERT_EQ(ProgramListResult::OK, result); - getListResult = ProgramListResult::NOT_READY; // continue as in NOT_READY case - } - if (getListResult == ProgramListResult::NOT_READY) { - EXPECT_TIMEOUT_CALL_WAIT(*mCallback, backgroundScanComplete, kFullScanTimeout); - - // second (last) try... - hidlResult = mTuner->getProgramList("", getListCb); - ASSERT_TRUE(hidlResult.isOk()); - ASSERT_EQ(ProgramListResult::OK, getListResult); - } - - if (isListEmpty) { - printSkipped("Program list is empty."); - return; - } + if (!getProgramList(getCb)) return; ProgramSelector selCb; EXPECT_CALL(*mCallback, tuneComplete(_, _)); @@ -296,6 +310,67 @@ TEST_P(BroadcastRadioHalTest, CancelAnnouncement) { EXPECT_EQ(Result::OK, hidlResult); } +/** + * Test getImage call with invalid image ID. + * + * Verifies that: + * - getImage call handles argument 0 gracefully + */ +TEST_P(BroadcastRadioHalTest, GetNoImage) { + if (skipped) return; + + size_t len = 0; + auto hidlResult = + mRadioModule->getImage(0, [&](hidl_vec rawImage) { len = rawImage.size(); }); + + ASSERT_TRUE(hidlResult.isOk()); + ASSERT_EQ(0u, len); +} + +/** + * Test proper image format in metadata. + * + * Verifies that: + * - all images in metadata are provided out-of-band (by id, not as a binary blob) + * - images are available for getImage call + */ +TEST_P(BroadcastRadioHalTest, OobImagesOnly) { + if (skipped) return; + ASSERT_TRUE(openTuner(0)); + + std::vector imageIds; + + ProgramInfo firstProgram; + auto getCb = [&](const hidl_vec& list) { + for (auto&& program : list) { + for (auto&& entry : program.base.metadata) { + EXPECT_NE(MetadataType::RAW, entry.type); + if (entry.key != MetadataKey::ICON && entry.key != MetadataKey::ART) continue; + EXPECT_NE(0, entry.intValue); + EXPECT_EQ(0u, entry.rawValue.size()); + if (entry.intValue != 0) imageIds.push_back(entry.intValue); + } + } + }; + if (!getProgramList(getCb)) return; + + if (imageIds.size() == 0) { + printSkipped("No images found"); + return; + } + + for (auto id : imageIds) { + ALOGD("Checking image %d", id); + + size_t len = 0; + auto hidlResult = + mRadioModule->getImage(id, [&](hidl_vec rawImage) { len = rawImage.size(); }); + + ASSERT_TRUE(hidlResult.isOk()); + ASSERT_GT(len, 0u); + } +} + INSTANTIATE_TEST_CASE_P(BroadcastRadioHalTestCases, BroadcastRadioHalTest, ::testing::Values(Class::AM_FM, Class::SAT, Class::DT)); diff --git a/current.txt b/current.txt index e85920fa76..a0cf6162c1 100644 --- a/current.txt +++ b/current.txt @@ -189,7 +189,7 @@ fe3c3c2f572b72f15f8594c538b0577bd5c28722c31879cfe6231330cddb6747 android.hardwar # ABI preserving changes to HALs released in Android O -8a4082dbc7f5eef585dca841b2656ba62d6c7e10e25dd05507ead15d96224f4c android.hardware.broadcastradio@1.0::types +150a338ce11fcec70757c9675d83cf6a5d7b40d0c812741b91671fecce59eac9 android.hardware.broadcastradio@1.0::types 760485232f6cce07f8bb05e3475509956996b702f77415ee5bff05e2ec5a5bcc android.hardware.dumpstate@1.0::IDumpstateDevice 1fecfa1609ff9d27ebf761a84b4336efa9d5dac5b241f19a6663f70d8db2c4b1 android.hardware.radio@1.0::IRadioResponse 28e929b453df3d9f5060af2764e6cdb123ddb893e3e86923c877f6ff7e5f02c9 android.hardware.wifi@1.0::types