Feature/vplay 10932 - move CachedFragment to separate compilation unit #484
Conversation
… new (mocked) L1 test for standalone testing.
test/utests/tests/CachedFragmentTests/CachedFragmentTestCases.cpp
Outdated
Show resolved
Hide resolved
test/utests/tests/CachedFragmentTests/CachedFragmentTestCases.cpp
Outdated
Show resolved
Hide resolved
test/utests/tests/CachedFragmentTests/CachedFragmentTestCases.cpp
Outdated
Show resolved
Hide resolved
test/utests/tests/CachedFragmentTests/CachedFragmentTestCases.cpp
Outdated
Show resolved
Hide resolved
test/utests/tests/CachedFragmentTests/CachedFragmentTestCases.cpp
Outdated
Show resolved
Hide resolved
| cachedFragment->fragment.AppendBytes(testData, testDataSize); | ||
|
|
||
| // Copy from empty source (sourceCachedFragment is default-initialized) | ||
| cachedFragment->Copy(sourceCachedFragment.get(), 0); |
There was a problem hiding this comment.
Hmm this looks wrong in the existing code, so not sure what we should be testing here.... consider copying into a non empty fragment. I don't think it will do what is expected. Testing current behaviour is unchanged is fine for now, but we should fix COPY going forward
| void CachedFragment::Copy(CachedFragment* other, size_t len) | ||
| { | ||
| // Clear existing data first | ||
| this->fragment.Free(); |
There was a problem hiding this comment.
I don't object, but this looks like its fixing a bug, and hence a change in behaviour ? From where its called that's fine, but suggest you make a note to check those places at later date as I think one of them calls clear first to avoid this issue, which is obviously no longer needed
| { | ||
| g_bufferStorage.clear(); | ||
| } | ||
|
|
||
| AampGrowableBuffer::~AampGrowableBuffer( void ) | ||
| { | ||
| if (g_mockAampGrowableBuffer) |
There was a problem hiding this comment.
This seems like an over complex solution ? instead of a map of vectors, surely is can just return a vector. Looks like it probably works so happy if you fix it in later delivery
| cachedFragment->cacheFragStreamInfo.reason = eAAMP_BITRATE_CHANGE_BY_TUNE; | ||
|
|
||
| // Add data to fragment buffer | ||
| cachedFragment->fragment.AppendBytes(testData, testDataSize); |
There was a problem hiding this comment.
probably want to remove this
Move CachedFragment + immediate dependencies to separate file.
Extend CachedFragment with idiomatic constructors.
Added CachedFragment to top level makefile.
Create L1 tests to verify function; extend fake AampGrowableBuffer to support these tests. In particular, allow for deep copy testing without needing to modify existing tests.
Modify existing L1 test makelists to ensure CachedFragment is built in.
NB - existing L1 tests ought to use a FakeCachedFragment, but that isn't part of this ticket.
Disabled a priv_aamp test that was causing nullptr dereference