From 2938b8238c4c937b18faf2595532cb60a4071712 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Mon, 25 May 2020 23:59:33 +0300 Subject: [PATCH 1/7] SL-12889 Failed to cache image crashes --- indra/newview/lltexturecache.cpp | 7 +++++-- indra/newview/lltexturefetch.cpp | 5 +++++ indra/newview/lltexturefetch.h | 1 + indra/newview/llviewertexture.cpp | 9 +++++++-- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/indra/newview/lltexturecache.cpp b/indra/newview/lltexturecache.cpp index 2e52414d71..6211d0ce3b 100644 --- a/indra/newview/lltexturecache.cpp +++ b/indra/newview/lltexturecache.cpp @@ -616,6 +616,9 @@ bool LLTextureCacheRemoteWorker::doWrite() if(idx >= 0) { // write to the fast cache. + // mRawImage is not entirely safe here since it is a pointer to one owned by cache worker, + // it could have been retrieved via getRequestFinished() and then modified. + // If writeToFastCache crashes, something is wrong around fetch worker. if(!mCache->writeToFastCache(mID, idx, mRawImage, mRawDiscardLevel)) { LL_WARNS() << "writeToFastCache failed" << LL_ENDL; @@ -2155,8 +2158,8 @@ bool LLTextureCache::writeToFastCache(LLUUID image_id, S32 id, LLPointer>= i; if(w * h *c > 0) //valid { - //make a duplicate to keep the original raw image untouched. - + // Make a duplicate to keep the original raw image untouched. + // Might be good idea to do a copy during writeToCache() call instead of here try { #if LL_WINDOWS diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp index fe058024f6..f64db7beb5 100644 --- a/indra/newview/lltexturefetch.cpp +++ b/indra/newview/lltexturefetch.cpp @@ -1977,6 +1977,11 @@ bool LLTextureFetchWorker::doWork(S32 param) setState(WAIT_ON_WRITE); ++mCacheWriteCount; CacheWriteResponder* responder = new CacheWriteResponder(mFetcher, mID); + // This call might be under work mutex, but mRawImage is not nessesary safe here. + // If something retrieves it via getRequestFinished() and modifies, image won't + // be protected by work mutex and won't be safe to use here nor in cache worker. + // So make sure users of getRequestFinished() does not attempt to modify image while + // fetcher is working mCacheWriteHandle = mFetcher->mTextureCache->writeToCache(mID, cache_priority, mFormattedImage->getData(), datasize, mFileSize, mRawImage, mDecodedDiscard, responder); diff --git a/indra/newview/lltexturefetch.h b/indra/newview/lltexturefetch.h index cdf8868597..2aa194e141 100644 --- a/indra/newview/lltexturefetch.h +++ b/indra/newview/lltexturefetch.h @@ -92,6 +92,7 @@ public: void deleteAllRequests(); // Threads: T* + // keep in mind that if fetcher isn't done, it still might need original raw image bool getRequestFinished(const LLUUID& id, S32& discard_level, LLPointer& raw, LLPointer& aux, LLCore::HttpStatus& last_http_get_status); diff --git a/indra/newview/llviewertexture.cpp b/indra/newview/llviewertexture.cpp index a2cec9a613..bd83a61e5b 100644 --- a/indra/newview/llviewertexture.cpp +++ b/indra/newview/llviewertexture.cpp @@ -1238,6 +1238,8 @@ void LLViewerFetchedTexture::loadFromFastCache() { if (mBoostLevel == LLGLTexture::BOOST_ICON) { + // Shouldn't do anything usefull since texures in fast cache are 16x16, + // it is here in case fast cache changes. S32 expected_width = mKnownDrawWidth > 0 ? mKnownDrawWidth : DEFAULT_ICON_DIMENTIONS; S32 expected_height = mKnownDrawHeight > 0 ? mKnownDrawHeight : DEFAULT_ICON_DIMENTIONS; if (mRawImage && (mRawImage->getWidth() > expected_width || mRawImage->getHeight() > expected_height)) @@ -1485,7 +1487,8 @@ BOOL LLViewerFetchedTexture::createTexture(S32 usename/*= 0*/) mOrigWidth = mRawImage->getWidth(); mOrigHeight = mRawImage->getHeight(); - + // This is only safe because it's a local image and fetcher doesn't use raw data + // from local images, but this might become unsafe in case of changes to fetcher if (mBoostLevel == BOOST_PREVIEW) { mRawImage->biasedScaleToPowerOfTwo(1024); @@ -1989,6 +1992,7 @@ bool LLViewerFetchedTexture::updateFetch() if (mRawImage.notNull()) sRawCount--; if (mAuxRawImage.notNull()) sAuxCount--; + // keep in mind that fetcher still might need raw image, don't modify original bool finished = LLAppViewer::getTextureFetch()->getRequestFinished(getID(), fetch_discard, mRawImage, mAuxRawImage, mLastHttpGetStatus); if (mRawImage.notNull()) sRawCount++; @@ -2048,7 +2052,8 @@ bool LLViewerFetchedTexture::updateFetch() if (mRawImage && (mRawImage->getWidth() > expected_width || mRawImage->getHeight() > expected_height)) { // scale oversized icon, no need to give more work to gl - mRawImage->scale(expected_width, expected_height); + // since we got mRawImage from thread worker and image may be in use (ex: writing cache), make a copy + mRawImage = mRawImage->scaled(expected_width, expected_height); } } From 293f5184811abdf5a9a41db490a58389dc3523fb Mon Sep 17 00:00:00 2001 From: Mnikolenko Productengine Date: Tue, 26 May 2020 17:45:01 +0300 Subject: [PATCH 2/7] SL-13279 Changes in Landmarks view UI - clarify that Notes are editable --- indra/newview/llpanellandmarkinfo.cpp | 9 +- indra/newview/llpanellandmarkinfo.h | 2 +- indra/newview/llpanelplaces.cpp | 15 +-- indra/newview/llpanelplaces.h | 1 - indra/newview/skins/default/colors.xml | 3 + .../default/xui/en/panel_landmark_info.xml | 21 ++++- .../skins/default/xui/en/panel_places.xml | 93 ++++++------------- 7 files changed, 64 insertions(+), 80 deletions(-) diff --git a/indra/newview/llpanellandmarkinfo.cpp b/indra/newview/llpanellandmarkinfo.cpp index 9b55fe9ce2..8a6a9f1fcd 100644 --- a/indra/newview/llpanellandmarkinfo.cpp +++ b/indra/newview/llpanellandmarkinfo.cpp @@ -78,7 +78,7 @@ BOOL LLPanelLandmarkInfo::postBuild() mCreator = getChild("creator"); mCreated = getChild("created"); - mLandmarkTitle = getChild("title_value"); + mLandmarkTitle = getChild("title_value"); mLandmarkTitleEditor = getChild("title_editor"); mNotesEditor = getChild("notes_editor"); mFolderCombo = getChild("folder_combo"); @@ -117,6 +117,7 @@ void LLPanelLandmarkInfo::setInfoType(EInfoType type) landmark_info_panel->setVisible(type == LANDMARK); getChild("folder_label")->setVisible(is_info_type_create_landmark); + getChild("edit_btn")->setVisible(!is_info_type_create_landmark); mFolderCombo->setVisible(is_info_type_create_landmark); switch(type) @@ -134,10 +135,6 @@ void LLPanelLandmarkInfo::setInfoType(EInfoType type) std::string name = parcel->getName(); LLVector3 agent_pos = gAgent.getPositionAgent(); - std::string desc; - LLAgentUI::buildLocationString(desc, LLAgentUI::LOCATION_FORMAT_FULL, agent_pos); - mNotesEditor->setText(desc); - if (name.empty()) { S32 region_x = ll_round(agent_pos.mV[VX]); @@ -152,6 +149,7 @@ void LLPanelLandmarkInfo::setInfoType(EInfoType type) } else { + std::string desc; LLAgentUI::buildLocationString(desc, LLAgentUI::LOCATION_FORMAT_NORMAL, agent_pos); region_name = desc; } @@ -349,6 +347,7 @@ void LLPanelLandmarkInfo::toggleLandmarkEditMode(BOOL enabled) mNotesEditor->setReadOnly(!enabled); mFolderCombo->setVisible(enabled); getChild("folder_label")->setVisible(enabled); + getChild("edit_btn")->setVisible(!enabled); // HACK: To change the text color in a text editor // when it was enabled/disabled we set the text once again. diff --git a/indra/newview/llpanellandmarkinfo.h b/indra/newview/llpanellandmarkinfo.h index 01a6fd6b3d..9712736182 100644 --- a/indra/newview/llpanellandmarkinfo.h +++ b/indra/newview/llpanellandmarkinfo.h @@ -71,7 +71,7 @@ private: LLTextBox* mOwner; LLTextBox* mCreator; LLTextBox* mCreated; - LLTextBox* mLandmarkTitle; + LLLineEditor* mLandmarkTitle; LLLineEditor* mLandmarkTitleEditor; LLTextEditor* mNotesEditor; LLComboBox* mFolderCombo; diff --git a/indra/newview/llpanelplaces.cpp b/indra/newview/llpanelplaces.cpp index 2ef82d0cf9..53870fb5c7 100644 --- a/indra/newview/llpanelplaces.cpp +++ b/indra/newview/llpanelplaces.cpp @@ -59,6 +59,7 @@ #include "llinventorymodel.h" #include "lllandmarkactions.h" #include "lllandmarklist.h" +#include "lllayoutstack.h" #include "llpanellandmarkinfo.h" #include "llpanellandmarks.h" #include "llpanelpick.h" @@ -280,9 +281,6 @@ BOOL LLPanelPlaces::postBuild() mShowOnMapBtn = getChild("map_btn"); mShowOnMapBtn->setClickedCallback(boost::bind(&LLPanelPlaces::onShowOnMapButtonClicked, this)); - mEditBtn = getChild("edit_btn"); - mEditBtn->setClickedCallback(boost::bind(&LLPanelPlaces::onEditButtonClicked, this)); - mSaveBtn = getChild("save_btn"); mSaveBtn->setClickedCallback(boost::bind(&LLPanelPlaces::onSaveButtonClicked, this)); @@ -355,6 +353,9 @@ BOOL LLPanelPlaces::postBuild() LLComboBox* folder_combo = mLandmarkInfo->getChild("folder_combo"); folder_combo->setCommitCallback(boost::bind(&LLPanelPlaces::onEditButtonClicked, this)); + LLButton* edit_btn = mLandmarkInfo->getChild("edit_btn"); + edit_btn->setCommitCallback(boost::bind(&LLPanelPlaces::onEditButtonClicked, this)); + createTabs(); updateVerbs(); @@ -532,7 +533,6 @@ void LLPanelPlaces::setItem(LLInventoryItem* item) BOOL is_landmark_editable = gInventory.isObjectDescendentOf(mItem->getUUID(), gInventory.getRootFolderID()) && mItem->getPermissions().allowModifyBy(gAgent.getID()); - mEditBtn->setEnabled(is_landmark_editable); mSaveBtn->setEnabled(is_landmark_editable); if (is_landmark_editable) @@ -1216,13 +1216,16 @@ void LLPanelPlaces::updateVerbs() mTeleportBtn->setVisible(!is_create_landmark_visible && !isLandmarkEditModeOn && !is_pick_panel_visible); mShowOnMapBtn->setVisible(!is_create_landmark_visible && !isLandmarkEditModeOn && !is_pick_panel_visible); - mOverflowBtn->setVisible(is_place_info_visible && !is_create_landmark_visible && !isLandmarkEditModeOn); - mEditBtn->setVisible(mPlaceInfoType == LANDMARK_INFO_TYPE && !isLandmarkEditModeOn); mSaveBtn->setVisible(isLandmarkEditModeOn); mCancelBtn->setVisible(isLandmarkEditModeOn); mCloseBtn->setVisible(is_create_landmark_visible && !isLandmarkEditModeOn); mPlaceInfoBtn->setVisible(!is_place_info_visible && !is_create_landmark_visible && !isLandmarkEditModeOn && !is_pick_panel_visible); + bool show_options_btn = is_place_info_visible && !is_create_landmark_visible && !isLandmarkEditModeOn; + mOverflowBtn->setVisible(show_options_btn); + getChild("lp_options")->setVisible(show_options_btn); + getChild("lp2")->setVisible(!show_options_btn); + mPlaceInfoBtn->setEnabled(!is_create_landmark_visible && !isLandmarkEditModeOn && have_3d_pos); if (is_place_info_visible) diff --git a/indra/newview/llpanelplaces.h b/indra/newview/llpanelplaces.h index 27f991c202..978b030b2e 100644 --- a/indra/newview/llpanelplaces.h +++ b/indra/newview/llpanelplaces.h @@ -121,7 +121,6 @@ private: LLButton* mPlaceProfileBackBtn; LLButton* mTeleportBtn; LLButton* mShowOnMapBtn; - LLButton* mEditBtn; LLButton* mSaveBtn; LLButton* mCancelBtn; LLButton* mCloseBtn; diff --git a/indra/newview/skins/default/colors.xml b/indra/newview/skins/default/colors.xml index e0da7f5d9e..e1cf708094 100644 --- a/indra/newview/skins/default/colors.xml +++ b/indra/newview/skins/default/colors.xml @@ -41,6 +41,9 @@ + diff --git a/indra/newview/skins/default/xui/en/panel_landmark_info.xml b/indra/newview/skins/default/xui/en/panel_landmark_info.xml index dd80f6eac8..e3b8e91c4f 100644 --- a/indra/newview/skins/default/xui/en/panel_landmark_info.xml +++ b/indra/newview/skins/default/xui/en/panel_landmark_info.xml @@ -277,9 +277,13 @@ top_pad="10" value="Title:" width="290" /> - +