[Merge] lp:~morphis/aethercast/add-lttng-support into lp:aethercast

[Konrad Zapałowicz]

Review: Needs Fixing

I noticed two things:

First the const TimestampUs is not passed as a reference. Second, I would prefer the consequence in using either the typedef or the raw type instead rather than mixing those.

Other than that it is a great merge.

Diff comments:

> 
> === added file 'src/mcs/report/lttng/packetizerreport.cpp'
> --- src/mcs/report/lttng/packetizerreport.cpp	1970-01-01 00:00:00 +0000
> +++ src/mcs/report/lttng/packetizerreport.cpp	2016-03-07 13:37:21 +0000
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (C) 2016 Canonical, Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU 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 warranties of
> + * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
> + * PURPOSE.  See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "mcs/report/lttng/packetizerreport.h"
> +
> +#define TRACEPOINT_DEFINE
> +#define TRACEPOINT_PROBE_DYNAMIC_LINKAGE
> +#include "mcs/report/lttng/packetizerreport_tp.h"
> +
> +namespace mcs {
> +namespace report {
> +namespace lttng {
> +
> +void PacketizerReport::PacketizedFrame(const TimestampUs timestamp) {

timestamp should be a reference.

> +    mcs_tracepoint(aethercast_packetizer, packetized_frame, timestamp);
> +}
> +
> +} // namespace lttng
> +} // namespace report
> +} // namespace mcs
> 
> === added file 'src/mcs/report/reportfactory.h'
> --- src/mcs/report/reportfactory.h	1970-01-01 00:00:00 +0000
> +++ src/mcs/report/reportfactory.h	2016-03-07 13:37:21 +0000
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright (C) 2016 Canonical, Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU 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 warranties of
> + * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
> + * PURPOSE.  See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef MCS_REPORT_REPORTFACTORY_H_
> +#define MCS_REPORT_REPORTFACTORY_H_
> +
> +#include <memory>
> +
> +#include "mcs/non_copyable.h"
> +
> +namespace mcs {
> +namespace video {
> +class EncoderReport;
> +class RendererReport;
> +class PacketizerReport;
> +class SenderReport;
> +} // namespace video
> +
> +namespace report {
> +
> +class ReportFactory : public mcs::NonCopyable {
> +public:
> +    static std::unique_ptr<ReportFactory> Create();
> +
> +    virtual std::shared_ptr<video::EncoderReport> CreateEncoderReport() = 0;

In many other places the video::EncoderXXX::Ptr is used instead a shared_ptr therefore shoudn't we keep the naming in sync. Personally I would prefer the shared_ptr version everywhere :)

> +    virtual std::shared_ptr<video::RendererReport> CreateRendererReport() = 0;
> +    virtual std::shared_ptr<video::PacketizerReport> CreatePacketizerReport() = 0;
> +    virtual std::shared_ptr<video::SenderReport> CreateSenderReport() = 0;
> +};
> +
> +} // namespace report
> +} // namespace mcs
> +
> +#endif


-- 
https://code.launchpad.net/~morphis/aethercast/add-lttng-support/+merge/288291
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~morphis/aethercast/add-lttng-support into lp:aethercast.