VPLAY-13071 Enforce move semantics on AampMediaSample send APIs#1205
Open
pstroffolino wants to merge 23 commits intodev_sprint_25_2from
Open
VPLAY-13071 Enforce move semantics on AampMediaSample send APIs#1205pstroffolino wants to merge 23 commits intodev_sprint_25_2from
pstroffolino wants to merge 23 commits intodev_sprint_25_2from
Conversation
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…r_to_vector_IDX_DRM_AampMediaSample_FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…r_to_vector_IDX_DRM_AampMediaSample_FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
VPLAY-13054 copilot-instructions.md update - c++11 -> c++17 Reason for Change: copilot generating warnings when we use c++17 syntax, despite aamp having been updated to build with C++17 tools by default. Changes instructions that required only c++ to be used, to avoid noise Test Guidance: check for impact with copilot feedback Risk: None --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…r_to_vector_IDX_DRM_AampMediaSample_FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…r_to_vector_IDX_DRM_AampMediaSample_FragmentCacheDescriptor
…, MediaStreamContext. DrmInterface, and FragmentCacheDescriptor
…r_to_vector_IDX_DRM_AampMediaSample_FragmentCacheDescriptor
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Reason for Change: Enforce call-site contract. Test Guidance: l1 tests passing Risk: Low- no functional change Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates AAMP’s sample injection and download helper APIs to enforce ownership-transfer semantics and remove ambiguous “optional output” pointer parameters, aligning implementations, mocks, fakes, and unit tests with the new contracts.
Changes:
- Make
AampMediaSamplemove-only and update sink/sample transfer APIs (SendSample/SendStreamTransfer) to takeAampMediaSample&&and move payloads. - Change
GetFile/LoadIDXoutput parameters from pointers to references and migrate IDX/key buffers tostd::vector<uint8_t>. - Update MPD/HLS collectors, demux paths, DRM key fetch, and unit tests/mocks/fakes to match the new signatures and buffer types.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp | Updates LoadIDX mock to fill a std::vector<uint8_t> IDX buffer (now needs deterministic http_code too). |
| test/utests/tests/PrivAampTests/PrivAampTests.cpp | Updates GetFile call sites to pass http_error by reference and adjusts a couple of test names/formatting. |
| test/utests/tests/Mp4BoxParsingTests/BoxParsingTests.cpp | Migrates sample data access from GetVector() to direct std::vector indexing. |
| test/utests/tests/AdManagerMPDTests/FunctionalTests.cpp | Updates helper/mocks to use int& httpError instead of int*. |
| test/utests/tests/AdFallbackTests/FunctionalTests.cpp | Updates helper signature to use int& httpError. |
| test/utests/tests/AampMp4DemuxTests/FunctionalTests.cpp | Updates SendStreamTransfer expectation to accept AampMediaSample&&. |
| test/utests/mocks/MockStreamSink.h | Updates SendSample mock to take AampMediaSample&&. |
| test/utests/mocks/MockPrivateInstanceAAMP.h | Updates GetFile, LoadIDX, and SendStreamTransfer mock signatures to reference/rvalue-ref forms. |
| test/utests/fakes/FakePrivateInstanceAAMP.cpp | Updates fake GetFile/LoadIDX signatures and moves AampMediaSample into the mock. |
| test/utests/fakes/FakeDrmInterface.cpp | Updates fake DRM interface constructor initialization. |
| test/utests/fakes/FakeAampGstPlayer.cpp | Updates fake SendSample signature to rvalue reference. |
| test/utests/drm/mocks/aampMocks.cpp | Updates DRM-side LoadIDX mock implementation to new buffer and http_code reference. |
| streamabstraction.cpp | Updates GetFile call to pass http_error by reference. |
| priv_aamp.h | Updates GetFile, LoadIDX, and SendStreamTransfer declarations and clarifies ownership/parameter docs. |
| priv_aamp.cpp | Implements the new GetFile / LoadIDX / SendStreamTransfer signatures and moves sample payload into the sink. |
| mp4demux/MP4Demux.cpp | Updates sample data insertion to operate directly on std::vector<uint8_t>. |
| mp4demux/AampMp4Demuxer.cpp | Moves AampMediaSample when calling SendStreamTransfer. |
| fragmentcollector_mpd.cpp | Replaces IDX buffer management with std::vector<uint8_t> and updates LoadIDX usage and cleanup. |
| fragmentcollector_hls.cpp | Updates GetFile calls to pass http_error by reference. |
| drm/DrmInterface.h | Replaces AES key buffer type with std::vector<uint8_t> (adds <vector> include). |
| drm/DrmInterface.cpp | Uses std::vector<uint8_t> for AES key downloads and only exposes key pointer on valid length. |
| admanager_mpd.cpp | Updates GetFile calls to pass http_error by reference. |
| aampgstplayer.h | Updates SendSample override signature and clarifies ownership contract in docs. |
| aampgstplayer.cpp | Moves AampMediaSample payload (mData, DRM metadata) into MediaSample for injection. |
| StreamSink.h | Updates SendSample to take AampMediaSample&& and documents ownership transfer. |
| MediaStreamContext.h | Replaces IDX with std::vector<uint8_t> and default-initializes fragment buffers. |
| MediaStreamContext.cpp | Updates GetFile call and buffer emptiness checks for std::vector-based storage. |
| FragmentCacheDescriptor.h | Replaces fragment download buffer pointer with a std::vector<uint8_t> member and updates docs. |
| AampStreamSinkInactive.h | Updates stub SendSample signature to match the new interface. |
| AampDemuxDataTypes.h | Replaces AampGrowableBuffer with std::vector<uint8_t> and makes AampMediaSample move-only. |
Comment on lines
2491
to
2495
| EXPECT_CALL(*g_mockPrivateInstanceAAMP, LoadIDX(_, _, _, _, _, _, _, _, _, _)) | ||
| .WillRepeatedly(WithArg<3>(Invoke([](AampGrowableBuffer *idxBuffer) | ||
| .WillRepeatedly(WithArg<3>(Invoke([](std::vector<uint8_t>& idxBuffer) | ||
| { | ||
| idxBuffer->AppendBytes((const uint8_t *)sidxBox, sizeof(sidxBox)); | ||
| idxBuffer.insert(idxBuffer.end(), std::cbegin(sidxBox), std::cend(sidxBox)); | ||
| }))); |
Comment on lines
+772
to
+796
| // In the absence of a mock, ensure output parameters are initialized | ||
| if (resetBuffer) | ||
| { | ||
| buffer.clear(); | ||
| } | ||
| http_error = 0; | ||
| effectiveUrl = remoteUrl; | ||
| if (downloadTime != nullptr) | ||
| { | ||
| *downloadTime = 0.0; | ||
| } | ||
| if (bitrate != nullptr) | ||
| { | ||
| *bitrate = 0; | ||
| } | ||
| if (fogError != nullptr) | ||
| { | ||
| *fogError = 0; | ||
| } | ||
|
|
||
| // Apply resetBuffer contract to keep fake semantics aligned with GetFile expectations | ||
| if (resetBuffer) | ||
| { | ||
| buffer.clear(); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates AAMP’s sample injection path to enforce ownership transfer via move semantics when sending AampMediaSample objects to sinks, aligning the API contract with how payloads are intended to be consumed.
Changes:
- Update
StreamSink::SendSampleandPrivateInstanceAAMP::SendStreamTransferto acceptAampMediaSample&&(consumed-on-call). - Update production call sites (e.g., MP4 demuxer → AAMP) to pass samples using
std::move. - Update unit tests, mocks, and fakes to match the new rvalue-reference signatures.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
StreamSink.h |
Changes SendSample to AampMediaSample&& and documents transfer-of-ownership semantics. |
aampgstplayer.h |
Updates AAMPGstPlayer::SendSample override to take AampMediaSample&&. |
aampgstplayer.cpp |
Updates implementation signature and continues moving sample payload into MediaSample. |
priv_aamp.h |
Updates PrivateInstanceAAMP::SendStreamTransfer sample overload signature and clarifies consumed-on-call semantics. |
priv_aamp.cpp |
Moves sample into sink (SendSample(..., std::move(sample))). |
mp4demux/AampMp4Demuxer.cpp |
Moves demuxed samples into SendStreamTransfer to comply with new contract. |
AampStreamSinkInactive.h |
Updates inactive sink stub SendSample signature to AampMediaSample&&. |
test/utests/mocks/MockStreamSink.h |
Updates mock SendSample signature to AampMediaSample&&. |
test/utests/mocks/MockPrivateInstanceAAMP.h |
Updates mock SendStreamTransfer (sample overload) signature to AampMediaSample&&. |
test/utests/fakes/FakePrivateInstanceAAMP.cpp |
Updates fake forwarding function to accept AampMediaSample&& and forward via std::move. |
test/utests/fakes/FakeAampGstPlayer.cpp |
Updates fake SendSample signature to AampMediaSample&&. |
test/utests/tests/AampMp4DemuxTests/FunctionalTests.cpp |
Updates test action signature to accept AampMediaSample&& for the mocked call. |
Comment on lines
94
to
+98
| * @param[in] mediaType - Type of the media. | ||
| * @param[in] sample - Media sample | ||
| * @param[in] sample - Media sample (consumed on call) | ||
| * @return void | ||
| */ | ||
| virtual bool SendSample( AampMediaType mediaType, AampMediaSample& sample ) = 0; | ||
| virtual bool SendSample( AampMediaType mediaType, AampMediaSample&& sample ) = 0; |
There was a problem hiding this comment.
The Doxygen for SendSample still says "@return void" but the method returns bool. Please update the documentation to describe the boolean return value (and what true/false means), especially since this change is clarifying ownership/consumption semantics.
| @@ -1351,7 +1351,7 @@ void AAMPGstPlayer::SetStreamCaps(AampMediaType type, MediaCodecInfo&& codecInfo | |||
| * @param[in,out] sample - Media sample to inject | |||
There was a problem hiding this comment.
The SendSample Doxygen comment still marks the sample parameter as "[in,out]", but the API now takes an rvalue reference and explicitly consumes the sample. Please update the comment to reflect that the sample is input-only and is moved/consumed by the call.
Suggested change
| * @param[in,out] sample - Media sample to inject | |
| * @param[in] sample - Media sample to inject; ownership is transferred and | |
| * its contents are moved/consumed by this call |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VPLAY-13071 Enforce move semantics on AampMediaSample send APIs
Reason for Change: enforce calling contract
Risk: Low (not functional change)
Test Guidance: basic regression only and L1 test passing