From 70efd0db0ae7443c76011f57b55cab99f66e9789 Mon Sep 17 00:00:00 2001 From: Dario Casalinuovo Date: Wed, 29 Jun 2016 18:30:53 +0200 Subject: [PATCH] MediaPlayer: Avoid to recreate the supplier more than needed * MediaPlayer attempted to recreate each time the media supplier(s), and implictly it recreated also the BMediaFile. This works fine with local data that can be accessed fastly, but makes lots of troubles with network streams such as BMediaIO. The result of this was that the Streamer plugin has been recreated each time having memory and network wasted other than performances. * I tried to keep intact the previous logic, and it looks OK for me, this has been done by moving out of the Controller the ownership of the TrackSupplier and adding a little utility class that do the releasing job previously done by the ObjectDeleter. * Reviews are appreciated. --- src/apps/mediaplayer/Controller.cpp | 97 ++++++++++++------- src/apps/mediaplayer/Controller.h | 1 - .../mediaplayer/playlist/FilePlaylistItem.cpp | 4 +- .../mediaplayer/playlist/FilePlaylistItem.h | 7 +- .../mediaplayer/playlist/PlaylistItem.cpp | 33 ++++++- src/apps/mediaplayer/playlist/PlaylistItem.h | 13 ++- .../mediaplayer/playlist/UrlPlaylistItem.cpp | 3 +- .../mediaplayer/playlist/UrlPlaylistItem.h | 5 +- 8 files changed, 109 insertions(+), 54 deletions(-) diff --git a/src/apps/mediaplayer/Controller.cpp b/src/apps/mediaplayer/Controller.cpp index f555cc8d89..76d839a6c6 100644 --- a/src/apps/mediaplayer/Controller.cpp +++ b/src/apps/mediaplayer/Controller.cpp @@ -53,6 +53,31 @@ using std::nothrow; +class TrackSupplierReleaser { +public: + TrackSupplierReleaser(PlaylistItemRef& owner) + : + fOwner(owner), + fRelease(true) + {} + + virtual ~TrackSupplierReleaser() + { + if (fRelease) + fOwner.Get()->ReleaseTrackSupplier(); + } + + void Detach() + { + fRelease = false; + } + +private: + PlaylistItemRef& fOwner; + bool fRelease; +}; + + void HandleError(const char *text, status_t err) { @@ -100,7 +125,6 @@ Controller::Controller() fMuted(false), fItem(NULL), - fTrackSupplier(NULL), fVideoSupplier(new ProxyVideoSupplier()), fAudioSupplier(new ProxyAudioSupplier(this)), @@ -235,13 +259,13 @@ Controller::SetTo(const PlaylistItemRef& item) return B_OK; } - fItem = item; - fAudioSupplier->SetSupplier(NULL, fVideoFrameRate); fVideoSupplier->SetSupplier(NULL); - ObjectDeleter oldTrackSupplierDeleter(fTrackSupplier); - fTrackSupplier = NULL; + if (fItem != NULL) + TrackSupplierReleaser oldSupplierReleaser(fItem); + + fItem = item; // Do not delete the supplier chain until after we called // NodeManager::Init() to setup a new media node chain @@ -266,12 +290,12 @@ Controller::SetTo(const PlaylistItemRef& item) if (fItem.Get() == NULL) return B_BAD_VALUE; - TrackSupplier* trackSupplier = fItem->CreateTrackSupplier(); + TrackSupplier* trackSupplier = fItem->GetTrackSupplier(); if (trackSupplier == NULL) { _NotifyFileChanged(item.Get(), B_NO_MEMORY); return B_NO_MEMORY; } - ObjectDeleter trackSupplierDeleter(trackSupplier); + TrackSupplierReleaser trackSupplierReleaser(fItem); status_t err = trackSupplier->InitCheck(); if (err != B_OK) { @@ -286,20 +310,17 @@ Controller::SetTo(const PlaylistItemRef& item) return B_MEDIA_NO_HANDLER; } - fTrackSupplier = trackSupplier; - SelectAudioTrack(0); SelectVideoTrack(0); if (fAudioTrackSupplier == NULL && fVideoTrackSupplier == NULL) { printf("Controller::SetTo: no audio or video tracks found or " "no decoders\n"); - fTrackSupplier = NULL; _NotifyFileChanged(item.Get(), B_MEDIA_NO_HANDLER); return B_MEDIA_NO_HANDLER; } - trackSupplierDeleter.Detach(); + trackSupplierReleaser.Detach(); // prevent blocking the creation of new overlay buffers if (fVideoView) @@ -423,8 +444,8 @@ Controller::AudioTrackCount() { BAutolock _(this); - if (fTrackSupplier != NULL) - return fTrackSupplier->CountAudioTracks(); + if (fItem != NULL && fItem->HasTrackSupplier()) + return fItem->GetTrackSupplier()->CountAudioTracks(); return 0; } @@ -434,8 +455,8 @@ Controller::VideoTrackCount() { BAutolock _(this); - if (fTrackSupplier != NULL) - return fTrackSupplier->CountVideoTracks(); + if (fItem != NULL && fItem->HasTrackSupplier()) + return fItem->GetTrackSupplier()->CountVideoTracks(); return 0; } @@ -445,8 +466,8 @@ Controller::SubTitleTrackCount() { BAutolock _(this); - if (fTrackSupplier != NULL) - return fTrackSupplier->CountSubTitleTracks(); + if (fItem != NULL && fItem->HasTrackSupplier()) + return fItem->GetTrackSupplier()->CountSubTitleTracks(); return 0; } @@ -455,11 +476,12 @@ status_t Controller::SelectAudioTrack(int n) { BAutolock _(this); - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; ObjectDeleter deleter(fAudioTrackSupplier); - fAudioTrackSupplier = fTrackSupplier->CreateAudioTrackForIndex(n); + fAudioTrackSupplier + = fItem->GetTrackSupplier()->CreateAudioTrackForIndex(n); if (fAudioTrackSupplier == NULL) return B_BAD_INDEX; @@ -505,11 +527,12 @@ Controller::SelectVideoTrack(int n) { BAutolock _(this); - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; ObjectDeleter deleter(fVideoTrackSupplier); - fVideoTrackSupplier = fTrackSupplier->CreateVideoTrackForIndex(n); + fVideoTrackSupplier + = fItem->GetTrackSupplier()->CreateVideoTrackForIndex(n); if (fVideoTrackSupplier == NULL) return B_BAD_INDEX; @@ -550,11 +573,12 @@ Controller::SelectSubTitleTrack(int n) { BAutolock _(this); - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; fSubTitlesIndex = n; - fSubTitles = fTrackSupplier->SubTitleTrackForIndex(n); + fSubTitles = + fItem->GetTrackSupplier()->SubTitleTrackForIndex(n); const SubTitle* subTitle = NULL; if (fSubTitles != NULL) @@ -586,10 +610,11 @@ Controller::SubTitleTrackName(int n) { BAutolock _(this); - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return NULL; - const SubTitles* subTitles = fTrackSupplier->SubTitleTrackForIndex(n); + const SubTitles* subTitles + = fItem->GetTrackSupplier()->SubTitleTrackForIndex(n); if (subTitles == NULL) return NULL; @@ -800,7 +825,7 @@ bool Controller::HasFile() { // you need to hold the data lock - return fTrackSupplier != NULL; + return fItem != NULL && fItem->HasTrackSupplier(); } @@ -808,9 +833,9 @@ status_t Controller::GetFileFormatInfo(media_file_format* fileFormat) { // you need to hold the data lock - if (!fTrackSupplier) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; - return fTrackSupplier->GetFileFormatInfo(fileFormat); + return fItem->GetTrackSupplier()->GetFileFormatInfo(fileFormat); } @@ -818,9 +843,9 @@ status_t Controller::GetCopyright(BString* copyright) { // you need to hold the data lock - if (!fTrackSupplier) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; - return fTrackSupplier->GetCopyright(copyright); + return fItem->GetTrackSupplier()->GetCopyright(copyright); } @@ -890,9 +915,9 @@ status_t Controller::GetMetaData(BMessage* metaData) { // you need to hold the data lock - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; - return fTrackSupplier->GetMetaData(metaData); + return fItem->GetTrackSupplier()->GetMetaData(metaData); } @@ -900,9 +925,9 @@ status_t Controller::GetVideoMetaData(int32 index, BMessage* metaData) { // you need to hold the data lock - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; - return fTrackSupplier->GetVideoMetaData(index, metaData); + return fItem->GetTrackSupplier()->GetVideoMetaData(index, metaData); } @@ -910,9 +935,9 @@ status_t Controller::GetAudioMetaData(int32 index, BMessage* metaData) { // you need to hold the data lock - if (fTrackSupplier == NULL) + if (fItem == NULL || !fItem->HasTrackSupplier()) return B_NO_INIT; - return fTrackSupplier->GetAudioMetaData(index, metaData); + return fItem->GetTrackSupplier()->GetAudioMetaData(index, metaData); } diff --git a/src/apps/mediaplayer/Controller.h b/src/apps/mediaplayer/Controller.h index 1eeda17f2f..e271cb17a7 100644 --- a/src/apps/mediaplayer/Controller.h +++ b/src/apps/mediaplayer/Controller.h @@ -197,7 +197,6 @@ private: bool fMuted; PlaylistItemRef fItem; - TrackSupplier* fTrackSupplier; ProxyVideoSupplier* fVideoSupplier; ProxyAudioSupplier* fAudioSupplier; diff --git a/src/apps/mediaplayer/playlist/FilePlaylistItem.cpp b/src/apps/mediaplayer/playlist/FilePlaylistItem.cpp index 976cff41c0..56d3cec3a3 100644 --- a/src/apps/mediaplayer/playlist/FilePlaylistItem.cpp +++ b/src/apps/mediaplayer/playlist/FilePlaylistItem.cpp @@ -288,7 +288,7 @@ FilePlaylistItem::RestoreFromTrash() // #pragma mark - TrackSupplier* -FilePlaylistItem::CreateTrackSupplier() const +FilePlaylistItem::_CreateTrackSupplier() const { MediaFileTrackSupplier* supplier = new(std::nothrow) MediaFileTrackSupplier(); @@ -400,7 +400,7 @@ FilePlaylistItem::ImageRef() const bigtime_t -FilePlaylistItem::_CalculateDuration() const +FilePlaylistItem::_CalculateDuration() { BMediaFile mediaFile(&Ref()); diff --git a/src/apps/mediaplayer/playlist/FilePlaylistItem.h b/src/apps/mediaplayer/playlist/FilePlaylistItem.h index 5ca3e431ca..82b6235664 100644 --- a/src/apps/mediaplayer/playlist/FilePlaylistItem.h +++ b/src/apps/mediaplayer/playlist/FilePlaylistItem.h @@ -52,9 +52,6 @@ public: virtual status_t MoveIntoTrash(); virtual status_t RestoreFromTrash(); - // playback - virtual TrackSupplier* CreateTrackSupplier() const; - status_t AddRef(const entry_ref& ref); const entry_ref& Ref() const { return fRefs[0]; } @@ -62,7 +59,9 @@ public: const entry_ref& ImageRef() const; protected: - virtual bigtime_t _CalculateDuration() const; + virtual bigtime_t _CalculateDuration(); + // playback + virtual TrackSupplier* _CreateTrackSupplier() const; private: status_t _SetAttribute(const char* attrName, diff --git a/src/apps/mediaplayer/playlist/PlaylistItem.cpp b/src/apps/mediaplayer/playlist/PlaylistItem.cpp index e3ec718dc1..e6c1003122 100644 --- a/src/apps/mediaplayer/playlist/PlaylistItem.cpp +++ b/src/apps/mediaplayer/playlist/PlaylistItem.cpp @@ -42,7 +42,8 @@ static vint32 sInstanceCount = 0; PlaylistItem::PlaylistItem() : - fPlaybackFailed(false) + fPlaybackFailed(false), + fTrackSupplier(NULL) { #ifdef DEBUG_INSTANCE_COUNT atomic_add(&sInstanceCount, 1); @@ -60,6 +61,31 @@ PlaylistItem::~PlaylistItem() } +TrackSupplier* +PlaylistItem::GetTrackSupplier() +{ + if (fTrackSupplier == NULL) + fTrackSupplier = _CreateTrackSupplier(); + + return fTrackSupplier; +} + + +void +PlaylistItem::ReleaseTrackSupplier() +{ + delete fTrackSupplier; + fTrackSupplier = NULL; +} + + +bool +PlaylistItem::HasTrackSupplier() const +{ + return fTrackSupplier != NULL; +} + + BString PlaylistItem::Name() const { @@ -160,10 +186,10 @@ PlaylistItem::_NotifyListeners() const } -bigtime_t PlaylistItem::_CalculateDuration() const +bigtime_t PlaylistItem::_CalculateDuration() { // To be overridden in subclasses with more efficient methods - TrackSupplier* supplier = CreateTrackSupplier(); + TrackSupplier* supplier = GetTrackSupplier(); AudioTrackSupplier* au = supplier->CreateAudioTrackForIndex(0); VideoTrackSupplier* vi = supplier->CreateVideoTrackForIndex(0); @@ -173,7 +199,6 @@ bigtime_t PlaylistItem::_CalculateDuration() const delete vi; delete au; - delete supplier; return duration; } diff --git a/src/apps/mediaplayer/playlist/PlaylistItem.h b/src/apps/mediaplayer/playlist/PlaylistItem.h index 5c7e94cf32..7331c21ade 100644 --- a/src/apps/mediaplayer/playlist/PlaylistItem.h +++ b/src/apps/mediaplayer/playlist/PlaylistItem.h @@ -90,8 +90,13 @@ public: virtual status_t MoveIntoTrash() = 0; virtual status_t RestoreFromTrash() = 0; - // playback - virtual TrackSupplier* CreateTrackSupplier() const = 0; + // Create and return the TrackSupplier if it doesn't exist, + // this object is used for media playback. + TrackSupplier* GetTrackSupplier(); + // Delete and reset the TrackSupplier + void ReleaseTrackSupplier(); + // Return whether the supplier has been initialized + bool HasTrackSupplier() const; void SetPlaybackFailed(); bool PlaybackFailed() const @@ -103,11 +108,13 @@ public: protected: void _NotifyListeners() const; - virtual bigtime_t _CalculateDuration() const; + virtual bigtime_t _CalculateDuration(); + virtual TrackSupplier* _CreateTrackSupplier() const = 0; private: BList fListeners; bool fPlaybackFailed; + TrackSupplier* fTrackSupplier; }; typedef BReference PlaylistItemRef; diff --git a/src/apps/mediaplayer/playlist/UrlPlaylistItem.cpp b/src/apps/mediaplayer/playlist/UrlPlaylistItem.cpp index a811fa9e00..d987ef676c 100644 --- a/src/apps/mediaplayer/playlist/UrlPlaylistItem.cpp +++ b/src/apps/mediaplayer/playlist/UrlPlaylistItem.cpp @@ -32,7 +32,6 @@ UrlPlaylistItem::UrlPlaylistItem(const BMessage* archive) UrlPlaylistItem::~UrlPlaylistItem() { - delete fUrl; } @@ -128,7 +127,7 @@ UrlPlaylistItem::RestoreFromTrash() TrackSupplier* -UrlPlaylistItem::CreateTrackSupplier() const +UrlPlaylistItem::_CreateTrackSupplier() const { MediaFileTrackSupplier* supplier = new(std::nothrow) MediaFileTrackSupplier(); diff --git a/src/apps/mediaplayer/playlist/UrlPlaylistItem.h b/src/apps/mediaplayer/playlist/UrlPlaylistItem.h index cf911f0bff..234f6d8e78 100644 --- a/src/apps/mediaplayer/playlist/UrlPlaylistItem.h +++ b/src/apps/mediaplayer/playlist/UrlPlaylistItem.h @@ -45,10 +45,11 @@ public: virtual status_t MoveIntoTrash(); virtual status_t RestoreFromTrash(); - virtual TrackSupplier* CreateTrackSupplier() const; - BUrl Url() const; +protected: + virtual TrackSupplier* _CreateTrackSupplier() const; + private: BUrl fUrl; };