RDKEMW-2731 : Bring Aamp changes for March sprint #15
Conversation
Reason for change: TO display aamp branch in devices Test Procedure: Updated in ticket Risks: Medium Priority: P1 Signed-off-by: Abhijith_Sasikumar <[email protected]>
Reason for change: remove temp code associated with lldash behaviors Test Procedure: Risks: medium Change-Id: I4903dda3bea606f70da6958b7a2c8aa161fe9d1b
Reason for change: Having AAMP TSB enabled breaks VOD. Changes: Restrict the enabling of AAMP TSB to live (ContentType_LINEAR) content only. Test Procedure: Confirm VOD behaves the same on FS regardless of if AAMP TSB is enabled in aamp.cfg Risks: Low Change-Id: I6f5911b61cd509132ca35a905669d20241d8b4ad Signed-off-by: Dominic Holden<[email protected]>
Reason for change: fix regression where install-aamp.sh (on osx) ends up trying to play bogus url Test Procedure: run install-aamp.sh; now just launches xcode without attempting to play an example.com locator Risks: Low Change-Id: I1f5717b5e8a9470064f8f7e74e9468122bf748f4
…n VOD DAI test stream Reason for change: Period end time also needs to be updated according to the PTO if PTO is greater than the start time. Test Procedure: Test case updated in the ticket Risks: low Change-Id: I0a551909d9984348cdb54d7935f7f9922b5bb4b4 Signed-off-by: [email protected]
Reason for change: No subtitles when playing SLD content from AAMP TSB Changes: * When reading from TSB, set the position to the PTS restamped value * Recalculate PTS for subtitle segments when writing to TSB * Add INFO to CacheFragment() with the position after restamp * Remove setting the position to absolute value when writing to TSB since AddFragment() is already setting it to absPosition. * Modify L1 AampTsbSessionManager tests to use AampUtils mocks. * Verify in L1 tests that the position read from TSB is restamped. Test Procedure: Verify subtitles for SLD linear content from TSB Risks: Low Change-Id: Id18b32c7e537a6c2a40dd6b14a80f8c82556d17e Signed-off-by: Jose Fagundez <[email protected]>
Reason for change: more predicable cdai sim in aampcli Test Procedure: refer ticket Risks: Low Change-Id: Ib7e4182c2c51dbf6771874ebf58d021a6bea97d6
Reason for change: avoid assertion if retuning to this type of HLS content Test Procedure: see ticket Risks: Low Change-Id: I25a09d7afeffde836a597fa62281d5d1fe9d0132
…and share notes with aamp team Reason for change: recommended changes for UVE doc from JSPP lead Test Procedure: eyes on doc - resolve final diff Risks: Low Change-Id: I631cf3ebed85878f0ffda6603788d44f915752c3
Reason for change: Cleanup member variables in class TsbFragmentData Test Procedure: Run L1 and L2 Test Risks: Low Change-Id: I85e61d83285ec4c1931abad7f913c781eea8d5df Signed-off-by: Vijendra Nagori <[email protected]>
Reason for change: AAMP fails to create TSB dir inside container Changes: * Create directories recursively and set the permission of the created directory to 777 * Create the Flush directory before locking the TSB Location; it makes the code simpler * Create files in Local TSB with 666 permissions * Add std::filesystem permissions and perms to the TSB FS interface and remove create_directories * Verify the changes in TSB L1 mocked tests * Verify permissions in TSB L1 real fs tests Test Procedure: Run AAMP L1 tests Risks: Medium Change-Id: I34c7c7ec069c43e19eb4b0c585f86f61c92949af Signed-off-by: Jose Fagundez <[email protected]>
Reason for change: fix regression from debouncing changes Test Procedure: see original ticket Risks: Low Change-Id: I27b012c566d72926eab8907ba0e0d6c2df5ff603
Reason for change: code simplifications Test Procedure: basic regression tests (HLS) Risks: Low Change-Id: Ib7e7f4fcd3eadb5d2378f825a2b4987679f103d5
Reason for change: support for playback of muxed hls/ts w/ pts restamping Test Procedure: refer ticket Risks: Low Change-Id: If8acb510e07385f8b44d4229f915e49b46c1be76
Reason for Change : pts-offset , GST_EVENT_CUSTOM_DOWNSTREAM Event is sentto the Rialto module from AAMP subtitlesource only after the pipeline state is changed to PLAYING (~after 5 sec) Test Procedure: Tune to FAST channel from the list 1001 - 1006, 1025, 1026,1031 - 1036 and monitor subtitle display Risks: Medium Change-Id: Ieed3bdfef791fa51f39784832fe346c34f6d1458 Signed-off-by: haripriya_molakalapalli <[email protected]>
Reason for change: If low bw timeout is done in fog, do not do it in aamp Test Procedure: See ticket Risks: Low Change-Id: I035ae1dbad79fb9b8ec498404375fa3bf92b4d51 Signed-off-by: <[email protected]>
Reason for change: Clarify the code and help further development Changes: * Set LocalAAMPTsbInjection flag at the beginning of TuneHelper() instead of the end, so this flag is true only when playing from TSB * Simplify conditions by checking a single flag * Add and improve comments * Use a single flag to check when playing from TSB to identify EOS * Refactor StreamAbstractionAAMP_MPD::Start() to make it simpler * Add L1 tests to verify the TSB flags with SLD and VOD content Test Procedure: Run AAMP L1 and L2 tests Risks: Medium Change-Id: Idd10973488eff62d97c91dd39ede6f072699dfcd Signed-off-by: Jose Fagundez <[email protected]>
Reason for change: Disable use of IARM inside containers Test Procedure: Priority: P2 Risks: Low Change-Id: Iddc246677aeeaa99063104d80fb00f147a200bec Signed-off-by: shripad bankar <[email protected]>
This reverts commit 0300ef14c9484014661bff7e7566aede8f6e934f. Reason for revert: Caused a regression when AAMP Local TSB is not enabled, making multiple L2 tests fail. Change-Id: Ic4b81e61ffe68fc5dff799ee0750ec730791353f
Reason for change: Clarify the code and help further development Changes: * Set LocalAAMPTsbInjection flag at the beginning of TuneHelper() instead of the end, so this flag is true only when playing from TSB * Simplify conditions by checking a single flag * Add and improve comments * Use a single flag to check when playing from TSB to identify EOS * Refactor StreamAbstractionAAMP_MPD::Start() to make it simpler * Add L1 tests to verify the TSB flags with SLD and VOD content Test Procedure: Run AAMP L1 and L2 tests Risks: Medium Change-Id: Icf369331b9f11200ce01cd36d7945414fd661587 Signed-off-by: Jose Fagundez <[email protected]>
…unreliable Reason For Change: When playing from TSB. On a period boundary the audio track is getting disabled. This manifests as an intermittent L2 test failure. A block of code is not needed. Test Proceedure: Verify tests 5001, 5001 are passing consistently Risk: Low Change-Id: I4f8bf62ee2e8c060e27374579f74c9296294efd4
Reason for change: CDAI code cleanup and enhancing L2 tests related to CDAI Test Procedure: Make sure all CDAI L2 80xx passes and there are no regressions with CDAI playback Risks: Low Change-Id: I09df69942371be20a1d3f9fd2cb8b96a596289bb Signed-off-by: Vinish K B <[email protected]>
Reason for change: remove bad spellings from whitelist Test Procedure: review whitelist (and corresponding aamp code) to further improve Risks: Low Change-Id: Ie0486b3de804eba2e505b71f1fc4205b011bd5e0
Reason for change: Fixing a crash in RemoveProbe, probes were not removed while changing the rate, causing crash while stopping the playback at trickplay Risks: Low Test Procedure: Test with 8003 test cases Priority: P1 Change-Id: I9519950b670dee9f3163985cc2ed3d943a00729a Signed-off-by: Nandakishor Udiyannur<[email protected]>
Reason for change: remove clutter with not needed ifdef WIN32 Test Procedure: light regression Risks: Low Change-Id: Iab15c8faf4b241844026273a1e97afd40edeee1f
Reason for change: Test the logging. Test Procedure: IP Playbacks. Risks: None Change-Id: Ia6344f282c518977d4ff5063c17b1a3ccf054f5a Signed-off-by: Paulpandian Mariappan <[email protected]>
Reason for change: In some of the AampTsb files, few
milestone logs are previously added as warnings;
convert them to milestone logs
Changes:
* Changed some of the warning logs to milestone logs in
AampTsbDataManager.cpp, AampTsbReader.cpp and
fragmentcollector_mpd.cpp
Test Procedure: Run AAMP L2 tests 5001 and 5002 and verify that the
required logs are changed to milestones
Risks: Low
Change-Id: I1e05c27368e82e58dba7ef84dab89cf3267cb781
Signed-off-by: Anjali Jacob <[email protected]>
Reason for change:
With local TSB enabled, pause was always performing a seek into the TSB buffer.
Changed so that:
- Pausing pauses the gstreamer pipeline
- Resume from already within the TSB just resumes the pipeline to playing
- Resume from Live performs a seek to the nearest fragment in the TSB (which can still result in a jump forward/backwards of a partial fragment)
Test Procedure:
Run AAMP L1 and L2 test 5001 & 5002
Tested at full stack
Risks: Low
Signed-off-by: Andrew Shepherd<[email protected]>
Change-Id: I0f933fa137441b91718f4ebcfe29e45800bc3c9c
Reason for Change : Trick Mode(2xFF),only video stream is available but due to faulty configuration of Rialto audio stream context, pipeline/Video Sink struck at Ready/Paused state resulting in L3 test failure. Test Procedure: Test with L3 Risks: Medium Change-Id: I32fb3fdd0213b5436e3f58ba26dfdc1325da6cc1 Signed-off-by: haripriya_molakalapalli <[email protected]>
Reason for Change : In case of Rialto Sink Subtitle switch is not working Test Procedure: Test with L3 Rialto test widget Risks: Medium Change-Id: I0f8a780946d74b32b45c455932e66a25f791ebb4 Signed-off-by: haripriya_molakalapalli <[email protected]>
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"manifestUrl" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""manifestUrl"")" instead of "manifestUrl". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"debugLevel" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""debugLevel"")" instead of "debugLevel". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. [Note: The source code implementation of the function has been overridden by a builtin model.] Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"callback" is copied in call to copy assignment for class "std::function<void (GstMediaType, bool, bool, bool &, bool &)>", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""callback"")" instead of "callback". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionAccessing "this->bufferStatus" without holding lock "MediaTrack.mutex". Elsewhere, "MediaTrack.bufferStatus" is written to with "MediaTrack.mutex" held 1 out of 1 times. Medium Impact, CWE-366 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Unchecked return valueCalling "wait_for" without checking return value (as is done elsewhere 6 out of 6 times). Medium Impact, CWE-252 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Dereference before null checkNull-checking "pInterfacePlayerRDK" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. Medium Impact, CWE-476 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"callback" is copied in call to copy assignment for class "std::function<void (bool, int)>", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""callback"")" instead of "callback". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Double unlock"~unique_lock" unlocks "lock" while it is unlocked. [Note: The source code implementation of the function has been overridden by a builtin model.] Medium Impact, CWE-765 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Double unlock"PushNextFragment" unlocks "pMediaStreamContext->mutex" while it is unlocked. Medium Impact, CWE-765 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionAccessing "this->m_enabled" without holding lock "TSProcessor.m_mutex". Elsewhere, "TSProcessor.m_enabled" is written to with "TSProcessor.m_mutex" held 2 out of 3 times. Medium Impact, CWE-366 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Double unlock"FetchFragment" unlocks "pMediaStreamContext->mutex" while it is unlocked. Medium Impact, CWE-765 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"busEvent" is passed-by-value as parameter to "BusEventData::BusEventData(BusEventData const &) /implicit =default/", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""busEvent"")" instead of "busEvent". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Check of thread-shared field evades lock acquisitionThread1 sets "format" to a new value. Now the two threads have an inconsistent view of "format" and updates to fields correlated with "format" may be lost. High Impact, CWE-543 How to fixGuard the modification of "format" and the read used to decide whether to modify "format" with the same set of locks. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"_name" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""_name"")" instead of "_name". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. [Note: The source code implementation of the function has been overridden by a builtin model.] Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. [Note: The source code implementation of the function has been overridden by a builtin model.] Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Use of auto that causes a copyUsing the "auto" keyword without an "&" causes the copy of an object of type "GstPlayerPriv::CallbackData". Low Impact, CWE-none How to fixUse "const auto &" instead of "auto". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Indefinite waitA wait is performed without ensuring that the condition is not already satisfied while holding lock "MediaTrack.subtitleMutex". This can cause a deadlock if the notification happens before the lock is acquired. High Impact, CWE-none How to fixAcquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Variable copied when it could be moved"busEvent" is passed-by-value as parameter to "BusEventData::BusEventData(BusEventData const &) /implicit =default/", when it could be moved instead. Low Impact, CWE-none How to fixUse "std::move(""busEvent"")" instead of "busEvent". Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Resource leak in objectAllocating memory by calling "new Configs". High Impact, CWE-401 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
| std::string prId, std::shared_ptr<TsbInitData> initData, uint32_t timeScale, double PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), position(position), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), | ||
| relativePosition(relativePos), timeScale(timeScale), PTSOffsetSec(PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), absolutePositionS(absolutePositionS), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"initData" is passed-by-value as parameter to "std::shared_ptr::shared_ptr(std::shared_ptr const &) /explicit =default/", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""initData"")" instead of "initData".
| { | ||
| if (videoDiscontinuityIndex[i].position < audioDiscontinuityIndex[i].position) | ||
| for( DiscontinuityIndexNode &audioDiscontinuity : audio->mDiscontinuityIndex ) |
There was a problem hiding this comment.
Coverity Issue - Structurally dead code
Since the loop increment "++;" is unreachable, the loop body will never execute more than once.
Medium Impact, CWE-561
UNREACHABLE
| std::string prId, std::shared_ptr<TsbInitData> initData, uint32_t timeScale, double PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), position(position), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), | ||
| relativePosition(relativePos), timeScale(timeScale), PTSOffsetSec(PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), absolutePositionS(absolutePositionS), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"prId" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""prId"")" instead of "prId".
| if(!mAdtoInsertInNextBreakVec.empty()) | ||
| { | ||
| mPlacementObj = setPlacementObj(mPlacementObj.pendingAdbrkId,abObj.endPeriodId); | ||
| // Remove the placement object that was placed completely | ||
| RemovePlacementObj(adBreakIdToRemove); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"adBreakIdToRemove" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""adBreakIdToRemove"")" instead of "adBreakIdToRemove".
| std::string prId, std::shared_ptr<TsbInitData> initData, uint32_t timeScale, double PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), position(position), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), | ||
| relativePosition(relativePos), timeScale(timeScale), PTSOffsetSec(PTSOffsetSec) | ||
| : TsbSegment(url, media, prId), absolutePositionS(absolutePositionS), duration(duration), mPTS(pts), isDiscontinuous(disc), initFragData(initData), |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"url" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""url"")" instead of "url".
|
|
||
| Flush( 1.0/*rate*/, 0/*start*/, -1/*stop*/, 0/*baseTime*/ ); | ||
|
|
||
| for( auto segmentInfo : segmentList ) |
There was a problem hiding this comment.
Coverity Issue - Use of auto that causes a copy
Using the "auto" keyword without an "&" causes the copy of an object of type "AppContext::LoadHLS(char const *, unsigned long, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)::SegmentInfo".
Low Impact, CWE-none
AUTO_CAUSES_COPY
How to fix
Use "const auto &" instead of "auto".
| @@ -8144,7 +8134,7 @@ void PrivateInstanceAAMP::ScheduleRetune(PlaybackErrorType errorType, AampMediaT | |||
| gLock.lock(); | |||
| if (this->mIsRetuneInProgress) | |||
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "this->mIsRetuneInProgress" without holding lock "gMutex". Elsewhere, "PrivateInstanceAAMP.mIsRetuneInProgress" is written to with "gMutex" held 2 out of 2 times (1 of these accesses strongly imply that it is necessary).
Medium Impact, CWE-366
MISSING_LOCK
No description provided.