[Merge] lp:~thomas-voss/media-hub/make-video-size-a-proper-type into lp:media-hub
Jim Hodapp
jim.hodapp at canonical.com
Mon Dec 1 21:28:17 UTC 2014
Review: Needs Fixing code
A few inline comments below. Please make sure to rebase with trunk.
Diff comments:
> === modified file 'include/core/media/player.h'
> --- include/core/media/player.h 2014-11-26 12:40:29 +0000
> +++ include/core/media/player.h 2014-11-26 12:40:29 +0000
> @@ -20,6 +20,8 @@
> #define CORE_UBUNTU_MEDIA_PLAYER_H_
>
> #include <core/media/track.h>
> +
> +#include <core/media/video/dimensions.h>
> #include <core/media/video/sink.h>
>
> #include <core/property.h>
> @@ -148,11 +150,7 @@
> virtual const core::Signal<int64_t>& seeked_to() const = 0;
> virtual const core::Signal<void>& end_of_stream() const = 0;
> virtual core::Signal<PlaybackStatus>& playback_status_changed() = 0;
> - /**
> - * Called when the video height/width change. Passes height and width as a bitmask with
> - * height in the upper 32 bits and width in the lower 32 bits (both unsigned values)
> - */
> - virtual const core::Signal<uint64_t>& video_dimension_changed() const = 0;
> + virtual const core::Signal<video::Dimensions>& video_dimension_changed() const = 0;
> protected:
> Player();
>
>
> === added file 'include/core/media/video/dimensions.h'
> --- include/core/media/video/dimensions.h 1970-01-01 00:00:00 +0000
> +++ include/core/media/video/dimensions.h 2014-11-26 12:40:29 +0000
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright © 2014 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +#ifndef CORE_UBUNTU_MEDIA_VIDEO_DIMENSIONS_H_
> +#define CORE_UBUNTU_MEDIA_VIDEO_DIMENSIONS_H_
> +
> +#include <cstdint>
> +
> +#include <tuple>
> +
> +namespace core
> +{
> +namespace ubuntu
> +{
> +namespace media
> +{
> +namespace video
> +{
> +namespace detail
> +{
> +enum class DimensionTag { width, height };
> +
> +template<DimensionTag Tag, typename IntegerType>
> +class IntWrapper
Since this is a public-facing header, can you please document how to use this class and give an example or two inline via a doxygen class comment?
> +{
> +public:
> + static_assert(std::is_integral<IntegerType>::value, "IntWrapper<> only supports integral types.");
> + typedef IntegerType ValueType;
> +
> + IntWrapper() : value{0} {}
> + template<typename AnyInteger>
> + explicit IntWrapper(AnyInteger value) : value{static_cast<ValueType>(value)} {}
> +
> + template<typename T = IntegerType>
> + T as() const
> + {
> + static_assert(std::is_arithmetic<T>::value, "as() only supports arithmetic types.");
> + return static_cast<T>(value);
> + }
> +
> +private:
> + ValueType value;
> +};
> +
> +template<DimensionTag Tag, typename IntegerType>
> +std::ostream& operator<<(std::ostream& out, IntWrapper<Tag, IntegerType> const& value)
> +{
> + out << value.template as<>();
I've not seen this syntax before. What is value.template actually doing here?
> + return out;
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator == (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() == rhs.template as<>();
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator != (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() != rhs.template as<>();
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator <= (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() <= rhs.template as<>();
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator >= (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() >= rhs.template as<>();
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator < (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() < rhs.template as<>();
> +}
> +
> +template<DimensionTag Tag, typename IntegerType>
> +inline bool operator > (IntWrapper<Tag, IntegerType> const& lhs, IntWrapper<Tag, IntegerType> const& rhs)
> +{
> + return lhs.template as<>() > rhs.template as<>();
> +}
> +} // namespace detail
> +
> +/** @brief The integer Height of a video. */
> +typedef detail::IntWrapper<detail::DimensionTag::height, std::uint32_t> Height;
> +/** @brief The integer Width of a video. */
> +typedef detail::IntWrapper<detail::DimensionTag::width, std::uint32_t> Width;
> +
> +/** @brief Height and Width of a video. */
> +typedef std::tuple<Height, Width> Dimensions;
> +}
> +}
> +}
> +}
> +
> +#endif // CORE_UBUNTU_MEDIA_VIDEO_DIMENSIONS_H_
>
> === modified file 'src/core/media/codec.h'
> --- src/core/media/codec.h 2014-10-10 01:24:43 +0000
> +++ src/core/media/codec.h 2014-11-26 12:40:29 +0000
> @@ -230,6 +230,46 @@
> }
> };
>
> +namespace helper
> +{
> +template<core::ubuntu::media::video::detail::DimensionTag tag, typename IntegerType>
> +struct TypeMapper<core::ubuntu::media::video::detail::IntWrapper<tag, IntegerType>>
> +{
> + constexpr static ArgumentType type_value()
> + {
> + return core::dbus::ArgumentType::uint32;
> + }
> + constexpr static bool is_basic_type()
> + {
> + return true;
> + }
> + constexpr static bool requires_signature()
> + {
> + return false;
> + }
> +
> + static std::string signature()
> + {
> + static const std::string s = TypeMapper<std::uint32_t>::signature();
> + return s;
> + }
> +};
> +}
> +
> +template<core::ubuntu::media::video::detail::DimensionTag tag, typename IntegerType>
> +struct Codec<core::ubuntu::media::video::detail::IntWrapper<tag, IntegerType>>
> +{
> + static void encode_argument(core::dbus::Message::Writer& out, const core::ubuntu::media::video::detail::IntWrapper<tag, IntegerType>& in)
> + {
> + out.push_uint32(in.template as<std::uint32_t>());
> + }
> +
> + static void decode_argument(core::dbus::Message::Reader& out, core::ubuntu::media::video::detail::IntWrapper<tag, IntegerType>& in)
> + {
> + in = core::ubuntu::media::video::detail::IntWrapper<tag, IntegerType>{out.pop_uint32()};
> + }
> +};
> +
> }
> }
>
>
> === modified file 'src/core/media/engine.h'
> --- src/core/media/engine.h 2014-11-26 12:40:29 +0000
> +++ src/core/media/engine.h 2014-11-26 12:40:29 +0000
> @@ -106,7 +106,7 @@
> virtual const core::Signal<void>& client_disconnected_signal() const = 0;
> virtual const core::Signal<void>& end_of_stream_signal() const = 0;
> virtual const core::Signal<core::ubuntu::media::Player::PlaybackStatus>& playback_status_changed_signal() const = 0;
> - virtual const core::Signal<uint32_t, uint32_t>& video_dimension_changed_signal() const = 0;
> + virtual const core::Signal<video::Dimensions>& video_dimension_changed_signal() const = 0;
>
> virtual void reset() = 0;
> };
>
> === modified file 'src/core/media/gstreamer/engine.cpp'
> --- src/core/media/gstreamer/engine.cpp 2014-11-03 18:00:48 +0000
> +++ src/core/media/gstreamer/engine.cpp 2014-11-26 12:40:29 +0000
> @@ -100,9 +100,9 @@
> end_of_stream();
> }
>
> - void on_video_dimension_changed(uint32_t height, uint32_t width)
> + void on_video_dimension_changed(const media::video::Dimensions& dimensions)
> {
> - video_dimension_changed(height, width);
> + video_dimension_changed(dimensions);
> }
>
> Private()
> @@ -163,12 +163,11 @@
> &Private::on_end_of_stream,
> this))),
> on_video_dimension_changed_connection(
> - playbin.signals.on_add_frame_dimension.connect(
> + playbin.signals.on_video_dimensions_changed.connect(
> std::bind(
> &Private::on_video_dimension_changed,
> this,
> - std::placeholders::_1,
> - std::placeholders::_2)))
> + std::placeholders::_1)))
> {
> }
>
> @@ -203,7 +202,7 @@
> core::Signal<void> client_disconnected;
> core::Signal<void> end_of_stream;
> core::Signal<media::Player::PlaybackStatus> playback_status_changed;
> - core::Signal<uint32_t, uint32_t> video_dimension_changed;
> + core::Signal<core::ubuntu::media::video::Dimensions> video_dimension_changed;
> };
>
> gstreamer::Engine::Engine() : d(new Private{})
> @@ -379,7 +378,7 @@
> return d->playback_status_changed;
> }
>
> -const core::Signal<uint32_t, uint32_t>& gstreamer::Engine::video_dimension_changed_signal() const
> +const core::Signal<core::ubuntu::media::video::Dimensions>& gstreamer::Engine::video_dimension_changed_signal() const
> {
> return d->video_dimension_changed;
> }
>
> === modified file 'src/core/media/gstreamer/engine.h'
> --- src/core/media/gstreamer/engine.h 2014-11-03 18:00:48 +0000
> +++ src/core/media/gstreamer/engine.h 2014-11-26 12:40:29 +0000
> @@ -62,7 +62,7 @@
> const core::Signal<void>& client_disconnected_signal() const;
> const core::Signal<void>& end_of_stream_signal() const;
> const core::Signal<core::ubuntu::media::Player::PlaybackStatus>& playback_status_changed_signal() const;
> - const core::Signal<uint32_t, uint32_t>& video_dimension_changed_signal() const;
> + const core::Signal<core::ubuntu::media::video::Dimensions>& video_dimension_changed_signal() const;
>
> void reset();
>
>
> === modified file 'src/core/media/gstreamer/playbin.h'
> --- src/core/media/gstreamer/playbin.h 2014-11-03 18:00:48 +0000
> +++ src/core/media/gstreamer/playbin.h 2014-11-26 12:40:29 +0000
> @@ -75,8 +75,6 @@
> bus{gst_element_get_bus(pipeline)},
> file_type(MEDIA_FILE_TYPE_NONE),
> video_sink(nullptr),
> - video_height(0),
> - video_width(0),
> on_new_message_connection(
> bus.on_new_message.connect(
> std::bind(
> @@ -369,19 +367,22 @@
> pipeline,
> ¤t,
> &pending,
> - state_change_timeout.count());
> + state_change_timeout.count());
> + break;
> + }
>
> - if (new_state == GST_STATE_PLAYING)
> + // The state change has to have been successful to make
> + // sure that we indeed reached the requested new state.
> + if (result && new_state == GST_STATE_PLAYING)
> {
> // Get the video height/width from the video sink
> - get_video_dimensions();
> + if (has_video_sink_with_height_and_width())
> + signals.on_video_dimensions_changed(get_video_dimensions());
> #ifdef DEBUG_GST_PIPELINE
> std::cout << "Dumping pipeline dot file" << std::endl;
> GST_DEBUG_BIN_TO_DOT_FILE((GstBin*)pipeline, GST_DEBUG_GRAPH_SHOW_ALL, "pipeline");
> #endif
> }
> - break;
> - }
>
> return result;
> }
> @@ -396,27 +397,26 @@
> ms.count() * 1000);
> }
>
> - void get_video_dimensions()
> - {
> - if (video_sink != nullptr && g_strcmp0(::getenv("CORE_UBUNTU_MEDIA_SERVICE_VIDEO_SINK_NAME"), "mirsink") == 0)
> + bool has_video_sink_with_height_and_width()
> + {
> + return video_sink != nullptr && g_strcmp0(::getenv("CORE_UBUNTU_MEDIA_SERVICE_VIDEO_SINK_NAME"), "mirsink") == 0;
> + }
> +
> + core::ubuntu::media::video::Dimensions get_video_dimensions()
> + {
> + if (not has_video_sink_with_height_and_width())
> + throw std::runtime_error{"Could not get the height/width of each video frame"};
> +
> + uint32_t video_height = 0, video_width = 0;
> + g_object_get (video_sink, "height", &video_height, nullptr);
> + g_object_get (video_sink, "width", &video_width, nullptr);
> + std::cout << "video_height: " << video_height << ", video_width: " << video_width << std::endl;
> +
> + return core::ubuntu::media::video::Dimensions
> {
> - g_object_get (video_sink, "height", &video_height, nullptr);
> - g_object_get (video_sink, "width", &video_width, nullptr);
> - std::cout << "video_height: " << video_height << ", video_width: " << video_width << std::endl;
> - signals.on_add_frame_dimension(video_height, video_width);
> - }
> - else
> - std::cerr << "Could not get the height/width of each video frame" << std::endl;
> - }
> -
> - int get_video_height() const
> - {
> - return video_height;
> - }
> -
> - int get_video_width() const
> - {
> - return video_width;
> + core::ubuntu::media::video::Height{video_height},
> + core::ubuntu::media::video::Width{video_width}
> + };
> }
>
> std::string get_file_content_type(const std::string& uri) const
> @@ -499,8 +499,6 @@
> MediaFileType file_type;
> SurfaceTextureClientHybris stc_hybris;
> GstElement* video_sink;
> - uint32_t video_height;
> - uint32_t video_width;
> core::Connection on_new_message_connection;
> bool is_seeking;
> struct
> @@ -513,9 +511,9 @@
> core::Signal<Bus::Message::Detail::StateChanged> on_state_changed;
> core::Signal<uint64_t> on_seeked_to;
> core::Signal<void> on_end_of_stream;
> - core::Signal<media::Player::PlaybackStatus> on_playback_status_changed;
> - core::Signal<media::Player::Orientation> on_orientation_changed;
> - core::Signal<uint32_t, uint32_t> on_add_frame_dimension;
> + core::Signal<core::ubuntu::media::Player::PlaybackStatus> on_playback_status_changed;
> + core::Signal<core::ubuntu::media::Player::Orientation> on_orientation_changed;
> + core::Signal<core::ubuntu::media::video::Dimensions> on_video_dimensions_changed;
> core::Signal<void> client_disconnected;
> } signals;
> };
>
> === modified file 'src/core/media/mpris/player.h'
> --- src/core/media/mpris/player.h 2014-11-26 12:40:29 +0000
> +++ src/core/media/mpris/player.h 2014-11-26 12:40:29 +0000
> @@ -34,12 +34,16 @@
> #include <core/dbus/types/object_path.h>
> #include <core/dbus/types/variant.h>
>
> +#include <core/dbus/types/stl/tuple.h>
> +
> #include <boost/utility/identity_type.hpp>
>
> #include <string>
> #include <tuple>
> #include <vector>
>
> +#include <cstdint>
> +
> namespace dbus = core::dbus;
>
> namespace mpris
> @@ -133,7 +137,7 @@
> DBUS_CPP_SIGNAL_DEF(Seeked, Player, std::int64_t)
> DBUS_CPP_SIGNAL_DEF(EndOfStream, Player, void)
> DBUS_CPP_SIGNAL_DEF(PlaybackStatusChanged, Player, core::ubuntu::media::Player::PlaybackStatus)
> - DBUS_CPP_SIGNAL_DEF(VideoDimensionChanged, Player, std::uint64_t)
> + DBUS_CPP_SIGNAL_DEF(VideoDimensionChanged, Player, core::ubuntu::media::video::Dimensions)
> };
>
> struct Properties
>
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2014-11-26 12:40:29 +0000
> +++ src/core/media/player_implementation.cpp 2014-11-26 12:40:29 +0000
> @@ -408,12 +408,9 @@
> playback_status_changed()(status);
> });
>
> - d->engine->video_dimension_changed_signal().connect([this](uint32_t height, uint32_t width)
> + d->engine->video_dimension_changed_signal().connect([this](const media::video::Dimensions& dimensions)
> {
> - uint64_t mask = 0;
> - // Left most 32 bits are for height, right most 32 bits are for width
> - mask = (static_cast<uint64_t>(height) << 32) | static_cast<uint64_t>(width);
> - video_dimension_changed()(mask);
> + video_dimension_changed()(dimensions);
> });
> }
>
>
> === modified file 'src/core/media/player_skeleton.cpp'
> --- src/core/media/player_skeleton.cpp 2014-11-26 12:40:29 +0000
> +++ src/core/media/player_skeleton.cpp 2014-11-26 12:40:29 +0000
> @@ -19,6 +19,7 @@
>
> #include "apparmor.h"
> #include "codec.h"
> +#include "engine.h"
> #include "player_skeleton.h"
> #include "player_traits.h"
> #include "property_stub.h"
> @@ -318,7 +319,7 @@
> remote_playback_status_changed->emit(status);
> });
>
> - video_dimension_changed.connect([remote_video_dimension_changed](uint64_t mask)
> + video_dimension_changed.connect([remote_video_dimension_changed](const media::video::Dimensions& mask)
Change mask to be dimensions to be consistent with your other code.
> {
> remote_video_dimension_changed->emit(mask);
> });
> @@ -327,7 +328,7 @@
> core::Signal<int64_t> seeked_to;
> core::Signal<void> end_of_stream;
> core::Signal<media::Player::PlaybackStatus> playback_status_changed;
> - core::Signal<uint64_t> video_dimension_changed;
> + core::Signal<media::video::Dimensions> video_dimension_changed;
> } signals;
>
> };
> @@ -607,12 +608,12 @@
> return d->signals.playback_status_changed;
> }
>
> -const core::Signal<uint64_t>& media::PlayerSkeleton::video_dimension_changed() const
> +const core::Signal<media::video::Dimensions>& media::PlayerSkeleton::video_dimension_changed() const
> {
> return d->signals.video_dimension_changed;
> }
>
> -core::Signal<uint64_t>& media::PlayerSkeleton::video_dimension_changed()
> +core::Signal<media::video::Dimensions>& media::PlayerSkeleton::video_dimension_changed()
> {
> return d->signals.video_dimension_changed;
> }
>
> === modified file 'src/core/media/player_skeleton.h'
> --- src/core/media/player_skeleton.h 2014-10-14 20:05:20 +0000
> +++ src/core/media/player_skeleton.h 2014-11-26 12:40:29 +0000
> @@ -73,7 +73,7 @@
> virtual const core::Signal<int64_t>& seeked_to() const;
> virtual const core::Signal<void>& end_of_stream() const;
> virtual core::Signal<PlaybackStatus>& playback_status_changed();
> - virtual const core::Signal<uint64_t>& video_dimension_changed() const;
> + virtual const core::Signal<video::Dimensions>& video_dimension_changed() const;
>
> protected:
> // All creation time arguments go here.
> @@ -109,7 +109,7 @@
>
> virtual core::Signal<int64_t>& seeked_to();
> virtual core::Signal<void>& end_of_stream();
> - virtual core::Signal<uint64_t>& video_dimension_changed();
> + virtual core::Signal<video::Dimensions>& video_dimension_changed();
>
> private:
> struct Private;
>
> === modified file 'src/core/media/player_stub.cpp'
> --- src/core/media/player_stub.cpp 2014-11-26 12:40:29 +0000
> +++ src/core/media/player_stub.cpp 2014-11-26 12:40:29 +0000
> @@ -163,17 +163,17 @@
> playback_status_changed(status);
> });
>
> - dbus.video_dimension_changed->connect([this](uint64_t mask)
> + dbus.video_dimension_changed->connect([this](const media::video::Dimensions dimensions)
> {
> std::cout << "VideoDimensionChanged signal arrived via the bus." << std::endl;
> - video_dimension_changed(mask);
> + video_dimension_changed(dimensions);
> });
> }
>
> core::Signal<int64_t> seeked_to;
> core::Signal<void> end_of_stream;
> core::Signal<media::Player::PlaybackStatus> playback_status_changed;
> - core::Signal<uint64_t> video_dimension_changed;
> + core::Signal<media::video::Dimensions> video_dimension_changed;
>
> struct DBus
> {
> @@ -417,7 +417,7 @@
> return d->signals.playback_status_changed;
> }
>
> -const core::Signal<uint64_t>& media::PlayerStub::video_dimension_changed() const
> +const core::Signal<media::video::Dimensions>& media::PlayerStub::video_dimension_changed() const
> {
> return d->signals.video_dimension_changed;
> }
>
> === modified file 'src/core/media/player_stub.h'
> --- src/core/media/player_stub.h 2014-11-26 12:40:29 +0000
> +++ src/core/media/player_stub.h 2014-11-26 12:40:29 +0000
> @@ -85,7 +85,7 @@
> virtual const core::Signal<int64_t>& seeked_to() const;
> virtual const core::Signal<void>& end_of_stream() const;
> virtual core::Signal<PlaybackStatus>& playback_status_changed();
> - virtual const core::Signal<uint64_t>& video_dimension_changed() const;
> + virtual const core::Signal<video::Dimensions>& video_dimension_changed() const;
>
> private:
> struct Private;
>
--
https://code.launchpad.net/~thomas-voss/media-hub/make-video-size-a-proper-type/+merge/242879
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list