[Merge] lp:~phablet-team/media-hub/fix-1449790 into lp:media-hub
Alfonso Sanchez-Beato
alfonso.sanchez-beato at canonical.com
Mon Feb 22 09:08:04 UTC 2016
Review: Needs Fixing
See comments below.
Diff comments:
>
> === modified file 'src/core/media/gstreamer/playbin.cpp'
> --- src/core/media/gstreamer/playbin.cpp 2015-12-16 16:55:51 +0000
> +++ src/core/media/gstreamer/playbin.cpp 2016-02-19 16:39:56 +0000
> @@ -633,17 +625,83 @@
> /* cancellable */ NULL, &error),
> g_object_unref);
> if (!info)
> - {
> - std::string error_str(error->message);
> - g_error_free(error);
> -
> - std::cout << "Failed to query the URI for the presence of video content: "
> - << error_str << std::endl;
> return std::string();
> - }
>
> - std::string content_type(g_file_info_get_attribute_string(
> + return std::string(g_file_info_get_attribute_string(
> info.get(), G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE));
> +}
> +
> +std::string gstreamer::Playbin::encode_uri(const std::string& uri) const
> +{
> + if (uri.empty())
> + return std::string();
> +
> + std::string encoded_uri;
> + media::UriCheck::Ptr uri_check{std::make_shared<media::UriCheck>(uri)};
> + const std::string uri_scheme {g_uri_parse_scheme(uri.c_str())};
String returned by g_uri_parse_scheme() should be freed.
> + // We have a URI and it is already percent encoded
> + if (!uri_scheme.empty() and uri_check->is_encoded())
> + {
> +#ifdef VERBOSE_DEBUG
> + std::cout << "Is a URI and is already percent encoded" << std::endl;
> +#endif
> + encoded_uri = uri;
> + }
> + // We have a URI but it's not already percent encoded
> + else if (!uri_scheme.empty() and !uri_check->is_encoded())
> + {
> +#ifdef VERBOSE_DEBUG
> + std::cout << "Is a URI and is not already percent encoded" << std::endl;
> +#endif
> + encoded_uri.assign(g_uri_escape_string(uri.c_str(),
Assign does a copy, the value returned by g_uri_escape_string() needs to be freed.
> + "!$&'()*+,;=:/?[]@", // reserved chars
> + TRUE)); // Allow UTF-8 chars
> + }
> + else // We have a path and not a URI. Turn it into a full URI and encode it
> + {
> + GError *error = nullptr;
> +#ifdef VERBOSE_DEBUG
> + std::cout << "Is a path and is not already percent encoded" << std::endl;
> +#endif
> + encoded_uri.assign(g_filename_to_uri(uri.c_str(), nullptr, &error));
string returned by g_filename_to_uri() needs to be freed
> + if (error != nullptr)
> + {
> + std::cerr << "Warning: failed to get actual track content type: " << error->message
> + << std::endl;
> + g_error_free(error);
> + return std::string("audio/video/");
> + }
> + encoded_uri.assign(g_uri_escape_string(uri.c_str(),
Assign does a copy, the value returned by g_uri_escape_string() needs to be freed.
> + "!$&'()*+,;=:/?[]@", // reserved chars
> + TRUE)); // Allow UTF-8 chars
> + }
> +
> + return encoded_uri;
> +}
> +
> +std::string gstreamer::Playbin::decode_uri(const std::string& uri) const
> +{
> + if (uri.empty())
> + return std::string();
> +
> + return std::string(g_uri_unescape_string(uri.c_str(), nullptr));
You need to free here the string returned by g_uri_unescape_string()
> +}
> +
> +std::string gstreamer::Playbin::get_file_content_type(const std::string& uri) const
> +{
> + if (uri.empty())
> + return std::string();
> +
> + const std::string encoded_uri{encode_uri(uri)};
> +
> + const std::string content_type {file_info_from_uri(encoded_uri)};
> + if (content_type.empty())
> + {
> + std::cerr << "Warning: failed to get actual track content type" << std::endl;
> + return std::string("audio/video/");
> + }
> +
> + std::cout << "Found content type: " << content_type << std::endl;
>
> return content_type;
> }
>
> === added file 'src/core/media/util/uri_check.h'
> --- src/core/media/util/uri_check.h 1970-01-01 00:00:00 +0000
> +++ src/core/media/util/uri_check.h 2016-02-19 16:39:56 +0000
> @@ -0,0 +1,131 @@
> +/*
> + *
> + * 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: Jim Hodapp <jim.hodapp at canonical.com>
> + *
> + */
> +
> +#ifndef URI_CHECK_H_
> +#define URI_CHECK_H_
> +
> +#include <gio/gio.h>
> +
> +#include <memory>
> +#include <iostream>
> +
> +namespace core
> +{
> +namespace ubuntu
> +{
> +namespace media
> +{
> +
> +class UriCheck
> +{
> +public:
> + typedef std::shared_ptr<UriCheck> Ptr;
> +
> + UriCheck()
> + : is_encoded_(false),
> + is_local_file_(false),
> + local_file_exists_(false)
> + {
> + }
> +
> + UriCheck(const std::string& uri)
> + : uri_(uri),
> + is_encoded_(is_encoded()),
> + is_local_file_(is_local_file()),
> + local_file_exists_(file_exists())
is_encoded_ and is_local_file_ are never read, only set. That does not make much sense. Either they, together with is_local_file_, should be removed, and we keep the public methods to do checks on the URI as needed, or we set them in the constructor and consider the class immutable (we return them in the corresponding function, and keep the code that set them private). I also favor removing the set() function.
Another option would be to make them cached values, but that seems to overcomplicate what should be a simple class.
> + {
> + }
> +
> + virtual ~UriCheck()
> + {
> + }
> +
> + void set(const std::string& uri)
> + {
> + if (uri.empty())
> + return;
> +
> + uri_ = uri;
> + is_encoded_ = is_encoded();
> + is_local_file_ = is_local_file();
> + local_file_exists_ = file_exists();
> + }
> +
> + void clear()
> + {
> + uri_.clear();
> + }
> +
> + bool is_encoded() const
> + {
> + if (uri_.empty())
> + return false;
> +
> + const std::string unescaped_uri{g_uri_unescape_string(uri_.c_str(), nullptr)};
You need to free here the string returned by g_uri_unescape_string(). Some check on returning NULL should be performed too, here and also in calls to g_uri_parse_scheme().
> + return unescaped_uri.length() < uri_.length();
> + }
> +
> + bool is_local_file() const
> + {
> + if (uri_.empty())
> + return false;
> +
> + const std::string uri_scheme {g_uri_parse_scheme(uri_.c_str())};
String returned by g_uri_parse_scheme() should be freed.
> + return uri_.at(0) == '/' or
> + (uri_.at(0) == '.' and uri_.at(1) == '/') or
> + uri_scheme == "file";
Is this the right check? Should not it be considered a local file unless scheme != "file"? You could specify a local file as "folder/file.mp4", for instance, and the function would return "false" incorrectly.
> + }
> +
> + bool file_exists() const
> + {
> + if (!is_local_file_)
> + return false;
> +
> + GError *error = nullptr;
> + // Open the URI and get the mime type from it. This will currently only work for
> + // a local file
> + std::unique_ptr<GFile, void(*)(void *)> file(
> + g_file_new_for_uri(uri_.c_str()), g_object_unref);
> + std::unique_ptr<GFileInfo, void(*)(void *)> info(
> + g_file_query_info(
> + file.get(), G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE ","
> + G_FILE_ATTRIBUTE_ETAG_VALUE, G_FILE_QUERY_INFO_NONE,
> + /* cancellable */ NULL, &error),
> + g_object_unref);
> +
> + std::cout << "File \"" << uri_ << "\" exists: " << (info.get() != nullptr) << std::endl;
> +
> + return info.get() != nullptr;
> + }
> +
> +protected:
> + UriCheck(const UriCheck&) = delete;
> + UriCheck& operator=(const UriCheck&) = delete;
> +
> +private:
> + std::string uri_;
> + bool is_encoded_;
> + bool is_local_file_;
> + bool local_file_exists_;
> +};
> +
> +}
> +}
> +}
> +
> +#endif // URI_CHECK_H_
--
https://code.launchpad.net/~phablet-team/media-hub/fix-1449790/+merge/285813
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list