-
Notifications
You must be signed in to change notification settings - Fork 2
create sort buckets for improving thumbnail performance #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| } | ||
|
|
||
| private fun findPrevKeyFrame(positionUs: Long, keyFrameTimestampsUs: ArrayList<Long>): Long { | ||
| for(i in 0 until keyFrameTimestampsUs.size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have a search method.
We should use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we are relying on search to request and add more keyframes if required.
which means We should not use keyFrameTimestampUs list.
And let's use proper variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list cannot have a name like keyFrameTimestampsUs
| } | ||
|
|
||
| private val stubs = ArrayDeque<Stub>() | ||
| private val bucketListMap = LinkedHashMap<Long, ArrayList<Stub>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can create the final list in runtime in the reorder method itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should maintain as minimal state as possible.
And in this case, we don't get a huge advantage by maintaining this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initially i had this inside only. but it didn't make sense creating new list for every request @sahilbajaj . that's why moved it to global
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will happen for every batch. And batch frequency is big enough.. like more than 1 sec on an average
And lists will be small in size.
It's not a considerable optimization for sure. Not worth it
| finalList.clear() | ||
|
|
||
| forEach { | ||
| val keyFrame = findPrevKeyFrame(it.positionUs, keyFrameTimestampsUs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findPrevKeyFrame can return -1.
Either we stop returning -1 from there.. or Handle -1 here, whichever is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
5c636eb to
1b827fe
Compare
| forEach { | ||
| val nextKeyFrameIndex = source.search(it.positionUs) | ||
| val previousKeyFrameUs = | ||
| source.keyFrameTimestampsUsList[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be replaced with another extension method.
like source.getKeyFrameAt(pos: Int)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, the list shouldn't be exposed outside.
1e8c8bc to
d24387f
Compare
No description provided.