feat: port media/event to CMake #1222

cosmin · 2023-06-28T17:12:56Z

Rebasing #1155 and applying some fixes. Had to comment out the mpd_notify_muxer_listener_unittest because it depends on MockMpdNotifier from mpd/base which has not been ported yet. Can bring this test back once that has been ported.

Related to #1047

packager/media/event/CMakeLists.txt

Co-authored-by: Cosmin Stejerean <cstejerean@meta.com>

cosmin · 2023-07-06T19:41:20Z

squashed all the changes

joeyparrish · 2023-07-07T17:14:04Z

We are getting Windows build errors, such as packager/mpd/base/mpd_notifier.h(55,54): warning C4100: 'container_id': unreferenced formal parameter

I believe this is the full list from the log, though I may have missed something:


D:\a\shaka-packager\shaka-packager\packager\media\event\hls_notify_muxer_listener.cc(45,10): error C2220: the following warning is treated as an error [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\hls_notify_muxer_listener.cc(45,10): warning C4100: 'is_initial_encryption_info': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\hls_notify_muxer_listener.cc(150,47): warning C4100: 'duration_seconds': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\hls_notify_muxer_listener.cc(266,60): warning C4100: 'cue_data': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
  mpd_notify_muxer_listener.cc
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(55,54): error C2220: the following warning is treated as an error [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(55,54): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(74,47): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(105,48): warning C4100: 'size': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(104,47): warning C4100: 'duration': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(103,48): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(35,33): warning C4100: 'iv': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(190,62): warning C4100: 'file_name': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(215,50): warning C4100: 'size': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(214,50): warning C4100: 'start_byte_offset': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(213,49): warning C4100: 'timestamp': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\mpd_notify_muxer_listener.cc(220,60): warning C4100: 'cue_data': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
  multi_codec_muxer_listener.cc
  muxer_listener_factory.cc
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(55,54): error C2220: the following warning is treated as an error [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(55,54): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(74,47): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(105,48): warning C4100: 'size': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(104,47): warning C4100: 'duration': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\.\packager/mpd/base/mpd_notifier.h(103,48): warning C4100: 'container_id': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
  muxer_listener_internal.cc
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(265,64): error C2220: the following warning is treated as an error [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(265,64): warning C4100: 'field_path': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(264,70): warning C4100: 'message2': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(264,45): warning C4100: 'message1': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(269,66): warning C4100: 'field_path': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(268,72): warning C4100: 'message2': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(268,47): warning C4100: 'message1': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(274,67): warning C4100: 'field_path': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(273,48): warning C4100: 'message2': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(272,48): warning C4100: 'message1': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(344,60): warning C4100: 'fields': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(342,66): warning C4100: 'message2': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(342,41): warning C4100: 'message1': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
  vod_media_info_dump_muxer_listener.cc
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(35,33): error C2220: the following warning is treated as an error [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(35,33): warning C4100: 'iv': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(92,58): warning C4100: 'start_time': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(91,69): warning C4100: 'file_name': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(106,57): warning C4100: 'size': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(105,57): warning C4100: 'start_byte_offset': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(104,56): warning C4100: 'timestamp': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(109,67): warning C4100: 'cue_data': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]
D:\a\shaka-packager\shaka-packager\packager\media\event\vod_media_info_dump_muxer_listener.cc(108,56): warning C4100: 'timestamp': unreferenced formal parameter [D:\a\shaka-packager\shaka-packager\build\packager\media\event\media_event.vcxproj]

cosmin · 2023-07-07T18:43:53Z

@joeyparrish from what I can tell this is because especially the notify functions take a bunch of parameters that a listener could do something with, but they aren't necessarily used. There are 3 solutions from what I can tell

comment out the parameter name like this

const std::vector<uint8_t>& key_id,
const std::vector<uint8_t>& /* iv */,

use the windows macro UNREFERENCED_PARAMETER() for each one of the unused parameters
disable treating unreferenced formal parameter as an error

preferences on how to proceed?

joeyparrish · 2023-07-07T21:09:28Z

There are 3 solutions from what I can tell

comment out the parameter name like this
const std::vector<uint8_t>& key_id,
const std::vector<uint8_t>& /* iv */,
use the windows macro UNREFERENCED_PARAMETER() for each one of the unused parameters

disable treating unreferenced formal parameter as an error

preferences on how to proceed?

In other places where there is an abstract interface whose concrete implementations don't use all parameters, we have used the macro UNUSED(varname); in the implementation. (Similar to option 2, defined in packager/macros.h.) For example, see CallbackFile::Seek:

bool CallbackFile::Seek(uint64_t position) {
  UNUSED(position);
  VLOG(1) << "CallbackFile does not support Seek().";
  return false;
}

In some other places, I believe some of the volunteers have chosen to comment out the parameter name as in option 1.

I would prefer to avoid option 3, but if options 1 and 2 become too onerous, I am open to discuss whatever makes the job doable.

cosmin · 2023-07-11T10:07:31Z

@joeyparrish I went ahead and added UNUSED to code except for 3rd party usage (protobuf) where I tried to disable the MSVC warning for it. Don't have an easy way to verify this other than waiting for signals here though.

joeyparrish · 2023-07-12T17:57:48Z

Don't have an easy way to verify this other than waiting for signals here though.

Yeah, it's a pain. I don't have multiple operating systems sitting around for development, either. It is often the case that I have to upload, wait for tests on GitHub, then fix build errors for Windows or Mac.

Sometimes, if the solution isn't obvious, I have to enable a flag on the repo to give me SSH access to the VM on a workflow failure, to debug it interactively.

joeyparrish · 2023-07-12T19:42:02Z

On Windows:

packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(265,64): error C2220: the following warning is treated as an error
packager\third_party\protobuf\source\src\google/protobuf/util/message_differencer.h(265,64): warning C4100: 'field_path': unreferenced formal parameter

If the content of third-party headers are going to set off these warnings when the headers are included, we may not have any other option than to disable that warning project-wide. (At least on MSVC.)

cosmin · 2023-07-12T19:55:38Z

I tried to disable that for protobuf though by setting /wd4100 # unreferenced formal parameter in third_party/protobuf/CMakeLists.txt however it doesn't seem like it took effect. I see some compile options set in third_party/protobuf/source/CMakeLists.txt. I'd expect that to inherit what we set above though. But I can try to add it there directly as well.

cosmin · 2023-07-12T19:56:57Z

Oh I see, because it's in the header we get the problem as soon as we include the header, so we need to disable C4100 everywhere (that imports protobuf at least).

…ludes protobuf

cosmin · 2023-07-12T19:59:39Z

Ok, I think I found the right place to fix this, in the definition of add_proto_library

packager/protobuf.cmake

joeyparrish · 2023-07-13T03:16:47Z

I'll merge this if the tests pass. This should unlock two more modules: media/formats/dvb, and media/formats/packed_audio.

joeyparrish · 2023-07-13T15:40:55Z

It looks like I need to rewrite the HTTP tests. They may be hanging on httpbin.org. I'll disable them for now.

cosmin · 2023-07-13T20:27:44Z

Yeah, the HTTP tests have also been giving me some grief locally, they seem to pass sometimes and fail sometimes, and take a long time to run. I've been resorting to mostly just running the unit test binary for the modules I'm changing as a result.

joeyparrish · 2023-07-13T23:24:05Z

I have a multi-PR fix for the http tests, which I've just finished uploading. It consists of #1230 (merged), #1231 (in review), and #1232 (in draft, depends on #1231).

cosmin force-pushed the cmake-media-event branch from e7e77c8 to 4b44a37 Compare June 28, 2023 17:13

cadubentzen mentioned this pull request Jun 30, 2023

feat: port media/event to CMake #1155

Closed

3 tasks

joeyparrish requested changes Jul 5, 2023

View reviewed changes

packager/media/event/CMakeLists.txt Show resolved Hide resolved

packager/media/event/CMakeLists.txt Outdated Show resolved Hide resolved

packager/media/event/CMakeLists.txt Outdated Show resolved Hide resolved

joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 5, 2023

cosmin force-pushed the cmake-media-event branch from 4b44a37 to 87d4e7f Compare July 6, 2023 19:36

cosmin requested a review from joeyparrish July 6, 2023 19:37

feat: port media/event to CMake

63cf78d

Co-authored-by: Cosmin Stejerean <cstejerean@meta.com>

cosmin force-pushed the cmake-media-event branch from 741c77a to 63cf78d Compare July 6, 2023 19:41

joeyparrish approved these changes Jul 7, 2023

View reviewed changes

fix unused formal parameter warnings

a96e30b

cosmin requested a review from joeyparrish July 11, 2023 10:54

joeyparrish removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 12, 2023

joeyparrish approved these changes Jul 12, 2023

View reviewed changes

add /wd4100 to ignore unused formal parameter for everything that inc…

08fc661

…ludes protobuf

cosmin added 2 commits July 12, 2023 22:13

apply the fix for third party build from crypto branch here as well

21e7b4c

fix unit test name

82a3a4c

joeyparrish requested changes Jul 12, 2023

View reviewed changes

packager/protobuf.cmake Outdated Show resolved Hide resolved

Fix protobuf warning comments

a0d7bab

joeyparrish approved these changes Jul 13, 2023

View reviewed changes

joeyparrish merged commit 8d3b2c6 into shaka-project:cmake Jul 13, 2023

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

github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: port media/event to CMake #1222

feat: port media/event to CMake #1222

cosmin commented Jun 28, 2023 •

edited by joeyparrish

Loading

cosmin commented Jul 6, 2023

joeyparrish commented Jul 7, 2023

cosmin commented Jul 7, 2023

joeyparrish commented Jul 7, 2023 •

edited

Loading

cosmin commented Jul 11, 2023

joeyparrish commented Jul 12, 2023

joeyparrish commented Jul 12, 2023

cosmin commented Jul 12, 2023

cosmin commented Jul 12, 2023

cosmin commented Jul 12, 2023

joeyparrish commented Jul 13, 2023

joeyparrish commented Jul 13, 2023

cosmin commented Jul 13, 2023

joeyparrish commented Jul 13, 2023

feat: port media/event to CMake #1222

feat: port media/event to CMake #1222

Conversation

cosmin commented Jun 28, 2023 • edited by joeyparrish Loading

cosmin commented Jul 6, 2023

joeyparrish commented Jul 7, 2023

cosmin commented Jul 7, 2023

joeyparrish commented Jul 7, 2023 • edited Loading

cosmin commented Jul 11, 2023

joeyparrish commented Jul 12, 2023

joeyparrish commented Jul 12, 2023

cosmin commented Jul 12, 2023

cosmin commented Jul 12, 2023

cosmin commented Jul 12, 2023

joeyparrish commented Jul 13, 2023

joeyparrish commented Jul 13, 2023

cosmin commented Jul 13, 2023

joeyparrish commented Jul 13, 2023

cosmin commented Jun 28, 2023 •

edited by joeyparrish

Loading

joeyparrish commented Jul 7, 2023 •

edited

Loading