feat: convert mpd module to cmake #1234

cosmin · 2023-07-14T13:50:31Z

packager/mpd/base/mpd_builder.cc

cosmin · 2023-07-14T13:54:08Z

packager/mpd/base/mpd_builder.h

@@ -32,6 +32,15 @@ class AdaptationSet;
 class MediaInfo;
 class Period;

+class Clock {


I created this class to replace the base::Clock and be able to override it for testing. Probably needs to be moved somewhere else from mpd_builder.h though, suggestions?

I had to create something similar already in Muxer in b1095f6. See packager/media/base/muxer.h.

We can always create a new folder to hold these things if we want to dedup them. I agree that this doesn't really belong in mpd.

moved it to a separate file. This is used in the end to print ISO-8601 dates so I think it's better to keep this in std::tm instead of uint64_t timestamps. I updated the MuxerClock to use this Clock abstraction as well.

cosmin · 2023-07-14T13:55:05Z

packager/mpd/base/mpd_builder_unittest.cc

+    std::tm local_time = utc_time;
+    std::time_t utc_time_t = std::mktime(&local_time);
+    std::time_t offset = utc_time_t - std::mktime(std::gmtime(&utc_time_t));
+    mock_time_ = std::chrono::system_clock::from_time_t(utc_time_t + offset);


had to jump through some hoops to be able to specify a std:tm in UTC for test injection. I think this works but perhaps there is a better way? The only better solutions I could find were C++20 though.

In muxer, I have the clock abstraction return uint64_t in seconds since 1970 UTC. (Essentially the same as time_t.) Depending on how it is consumed, you could do the same here, and avoid time_point and std::chrono.

cosmin · 2023-07-14T13:55:58Z

packager/mpd/base/mpd_builder_unittest.cc

-    mpd_.InjectClockForTesting(std::unique_ptr<base::Clock>(
-        new TestClock(base::Time::FromUTCExploded(test_time))));
+    std::tm test_time{};
+    test_time.tm_year = 2016 - 1900;  // Years since 1900


I don't like that this is the best option I could find to initialize a test time from something human readable. Worth adding some kind of helper method for this? Or is there some better solution?

I tend to find ISO 8601 time strings to be pretty readable. If there's an easy way to parse those into the appropriate value type, I would use that. Ex: "2016-01-11T15:10:24Z"

ok, updated TestClock to use ISO-8601 format as the input

cosmin · 2023-07-14T14:00:14Z

packager/mpd/base/xml/xml_node.cc

+          absl::big_endian::FromHost16(codec_data.channel_mask());
+      uint8_t byte1 = ec3_channel_map & 0xFF;
+      uint8_t byte2 = ec3_channel_map >> 8;
+      audio_channel_config_value = absl::StrFormat("%02X%02X", byte1, byte2);


couldn't find a better way to get the base::HexEncode here in big endian format, is this the best way to proceed?

Maybe add another helper to packager/utils/hex_parser. I'm not certain you need to decompose it into individual bytes if you've already done a swap with absl. Just FromHost16 and StrFormat("%04X") should be fine, right? Or FromHost16 and BytesToHexString?

That's where I started and that didn't work, the byte order was reversed, but looks like I was overthinking it. StrFormat("%04X", ...) works by itself, without FromHost16, going to big endian order is only important if we go byte at a time like the manual formatting.

cosmin · 2023-07-14T14:00:56Z

packager/mpd/base/xml/xml_node.cc

      audio_channel_config_value =
-        base::HexEncode(&ac4_channel_mask, sizeof(ac4_channel_mask) - 1);
+          absl::StrFormat("%02X%02X%02X", bytes[0], bytes[1], bytes[2]);


likewise here for the uint32_t, perhaps I should the same approach for both AC3 and AC4 though.

Same as above, I think. Swap and print/format/hexify without needing Span or individual bytes.

Also, it took me a while to see the "- 1" on the original. Please add a comment about us needing just 3 bytes.

packager/mpd/test/mpd_builder_test_helper.cc

packager/mpd/test/xml_compare.cc

cosmin · 2023-07-14T22:54:46Z

mpd\base\bandwidth_estimator.cc(109,12): warning C4244: 'return': conversion from 'double' to 'uint64_t', possible loss of data

bandwidth_estimator.cc is throwing an error on windows due to narrowing conversion from double to uint64. Since this one seems quite intentional, what is the right way to do this without tripping on C4244?

joeyparrish · 2023-07-26T17:13:53Z

bandwidth_estimator.cc is throwing an error on windows due to narrowing conversion from double to uint64. Since this one seems quite intentional, what is the right way to do this without tripping on C4244?

Is this the return 0.0; line? That's easy, just replace with return 0. There's no reason that should be a floating-point literal in a function returning uint64_t.

joeyparrish · 2023-08-04T17:04:34Z

packager/mpd/base/mpd_utils_unittest.cc

-    std::vector<uint8_t> pssh;
-    base::HexStringToBytes(pssh_str, &pssh);
+
+    std::string pssh_hex_str = absl::HexStringToBytes(pssh_str);


I'd still like to clean this up.

packager/mpd/base/representation_unittest.cc

packager/mpd/base/xml/xml_node_unittest.cc

joeyparrish · 2023-08-04T17:13:16Z

packager/mpd/base/xml/xml_node_unittest.cc

    media_info_.set_segment_template_url("$Number$.m4s");
  }

-  void TearDown() override { FLAGS_segment_template_constant_duration = false; }
+  void TearDown() override {
+    absl::SetFlag(&FLAGS_segment_template_constant_duration, false);


This becomes unnecessary with FlagSaver.

packager/mpd/base/xml/xml_node_unittest.cc

joeyparrish · 2023-08-04T17:13:46Z

packager/mpd/base/xml/xml_node_unittest.cc

@@ -549,7 +550,7 @@ TEST_F(LiveSegmentTimelineTest, LastSegmentNumberSupplementalProperty) {
                   "  <SegmentTemplate media=\"$Number$.m4s\" "
                   "                   startNumber=\"1\" duration=\"100\"/>"
                   "</Representation>"));
-  FLAGS_dash_add_last_segment_number_when_needed = false;
+  absl::SetFlag(&FLAGS_dash_add_last_segment_number_when_needed, false);


This becomes unnecessary.

…library type with explicit source list

cosmin commented Jul 14, 2023

View reviewed changes

cosmin force-pushed the cmake-mpd branch 2 times, most recently from ef81fe5 to 7a83ad5 Compare July 14, 2023 22:24

cosmin mentioned this pull request Jul 17, 2023

feat: port HLS module to CMake #1238

Merged

cosmin requested a review from joeyparrish July 17, 2023 14:14

cosmin mentioned this pull request Jul 17, 2023

test: Fix data directory computation #1239

Merged

cosmin force-pushed the cmake-mpd branch 2 times, most recently from 0418c29 to b1234b8 Compare July 21, 2023 10:43

cosmin force-pushed the cmake-mpd branch 4 times, most recently from 48de724 to 25e89b2 Compare July 31, 2023 00:44

joeyparrish requested changes Aug 4, 2023

View reviewed changes

cosmin and others added 14 commits August 4, 2023 14:18

feat: convert mpd module to cmake

bc2aeee

make some fixes

d46700a

remove another unnecessary std:mmove

04b77ef

add missing LibXml2 linking for mpd mocks

28b5289

write a custom double to string formatting function to trim trailing 0s

f46a935

remove use of __FILE__ for test data

985a230

use std::filesystem::relative instead

ca12592

move Clock class to utils and move test setup to use ISO-8601 formatting

f3ae84b

fix accidental float return in BandwidthEstimator::GetBitrate

5a2527f

simplify hex formatting for AC3/AC4

33fcb5f

apply clang-format

667658c

move GetPathContent to test_data_util

487272e

support CMake older than 3.19 which doesn't allow defining INTERFACE …

bdf4cfd

…library type with explicit source list

include not import

57591f0

cosmin added 2 commits August 4, 2023 14:18

use POSIX separators for MakePathRelative

ba06383

use FlagSaver

54eed0e

cosmin force-pushed the cmake-mpd branch from 500656a to 54eed0e Compare August 4, 2023 21:43

cosmin added 2 commits August 4, 2023 14:46

cleanup pssh test in ContentProtectionPlayReadyCencMspr

361ae70

linting

f5adee0

cosmin requested a review from joeyparrish August 4, 2023 21:48

joeyparrish approved these changes Aug 4, 2023

View reviewed changes

cosmin merged commit 96acd1e into shaka-project:cmake Aug 5, 2023

cosmin deleted the cmake-mpd branch August 5, 2023 03:45

github-actions bot added the status: archived Archived and locked; will not be updated label Oct 4, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert mpd module to cmake #1234

feat: convert mpd module to cmake #1234

cosmin commented Jul 14, 2023 •

edited by joeyparrish

Loading

cosmin Jul 14, 2023

joeyparrish Jul 26, 2023

cosmin Jul 28, 2023

cosmin Jul 14, 2023

joeyparrish Jul 26, 2023

cosmin Jul 14, 2023

joeyparrish Jul 26, 2023

cosmin Jul 28, 2023

cosmin Jul 14, 2023

joeyparrish Jul 26, 2023

cosmin Jul 28, 2023

cosmin Jul 14, 2023

joeyparrish Jul 26, 2023

cosmin Jul 28, 2023

cosmin commented Jul 14, 2023 •

edited

Loading

joeyparrish commented Jul 26, 2023

joeyparrish Aug 4, 2023

joeyparrish Aug 4, 2023

joeyparrish Aug 4, 2023

feat: convert mpd module to cmake #1234

feat: convert mpd module to cmake #1234

Conversation

cosmin commented Jul 14, 2023 • edited by joeyparrish Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmin commented Jul 14, 2023 • edited Loading

joeyparrish commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmin commented Jul 14, 2023 •

edited by joeyparrish

Loading

cosmin commented Jul 14, 2023 •

edited

Loading