[Merge] lp:~phablet-team/media-hub/media-hub-desktop into lp:media-hub

Konrad Zapałowicz konrad.zapalowicz at canonical.com
Fri Aug 12 08:13:56 UTC 2016


Review: Needs Information code

LGTM except that I need an explanation if it is possible to cache get_backend_type()

Diff comments:

> 
> === added file 'src/core/media/backend.cpp'
> --- src/core/media/backend.cpp	1970-01-01 00:00:00 +0000
> +++ src/core/media/backend.cpp	2016-08-11 19:37:36 +0000
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2016 Canonical Ltd
> + *
> + * 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.
> + *
> + * Authored by: Jim Hodapp <jim.hodapp at canonical.com>
> + */
> +
> +#include "backend.h"
> +#include "core/media/logger/logger.h"
> +
> +#include <core/posix/signal.h>
> +
> +#include <cstring>
> +
> +namespace media = core::ubuntu::media;
> +
> +media::AVBackend::Backend media::AVBackend::get_backend_type()
> +{
> +    const char *backend = ::getenv("MH_BACKEND");

Could we cache this operation so that it is not required to getenv, strcmp each time the get_backend_type() is called. As I see it in the code below it is called in X_observer functions and as far as I understand the backend will not change for the platform, right?

I would also prefer strncmp over strcmp :)

> +    if (backend)
> +    {
> +        MH_DEBUG("MH_BACKEND: %s", backend);
> +        if (strcmp(backend, "hybris") == 0)
> +            return media::AVBackend::Backend::hybris;
> +        else
> +            return media::AVBackend::Backend::none;
> +    }
> +    else
> +    {
> +        return media::AVBackend::Backend::none;
> +    }
> +}


-- 
https://code.launchpad.net/~phablet-team/media-hub/media-hub-desktop/+merge/302712
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.



More information about the Ubuntu-reviews mailing list