-
Notifications
You must be signed in to change notification settings - Fork 253
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
Improved scene detection #2710
Improved scene detection #2710
Conversation
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.
Hi,
Thanks for the patch, 57 commits for this is nto easy to review, please squash the relevant commits, so reviewers can look it in better way
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.
Please squash and rebase on top of the current tree.
My primary concern is that this seems to remove the scene flash detection. This was used for both fast and normal (cost-based) detection and is based on what x264 does to try and avoid an extra keyframe for a very short (5-frame-or-less) scene, preferring to just put one keyframe after the flash. In the past, it also helped to alleviate some false positives with pans being detected as scenecuts using the old fast method--maybe that issue has been resolved with the new fast algorithm? |
@shssoichiro At this moment, new algorithm should handle pans just as fine as it use dequeue of scores to adjust threshold, which should also handle high motion / fade-in / fade-out Making score dequeue bidirectional (compare score for current frame to scores before and after) should fix the issue flash detection, and should give free speed boost for all speeds as it reduce amount of computation |
0b0308b
to
88d185a
Compare
88d185a
to
4b23924
Compare
Can you please rebase this? :) |
629292f
to
0828d1f
Compare
It's wip) it will crash) |
50759ec
to
a653e4c
Compare
4d1b7de
to
fbff89b
Compare
809fea8
to
3984fa8
Compare
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.
I did not check the code in-depth, was seeing the general implementation, I have some suggestions before landing
- The documentation for various things are good, but I felt the linebreaks are not consistent, sometimes happening after 20/30words, sometimes at 40/60, would be nice to be more consistent
- The commit logs, would require a cleanup of commits to see by splitting into two/three, and giving a gist of the changes int eh commit messages, maybe could see the latest PR from @shssoichiro where he was giving a good explanation of things either in the commit log or the PR.
Apart from that It is good in shape generally. Thanks for the work.
@lu-zero would be nice if you can take a close-look in the rust part of APIs :)
src/scenechange/mod.rs
Outdated
) | ||
} | ||
// Initially fill score deque with forward frames | ||
// ititiallization is different depending on frame set length |
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.
// ititiallization is different depending on frame set length | |
// Initialization is different depending on frame set length |
Need to fix the typo+ maybe a rewording is a good idea, like merging both sentences?
Initially fill score deque with forward frames based on frame_set length
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.
Fixed typos
src/scenechange/mod.rs
Outdated
debug!("[SC-score-deque]{:.0?}", self.score_deque); | ||
self.score_deque.clear(); | ||
} else { | ||
// Keep score deque 5 + lookahead_size frames |
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.
I wonder how did we reach 5+ lookahead_size, would be nice to add in commit message or here, whichever is better.
On seeing further code, could see the mention of 5 logic, would be better if it is mentioned here than later.
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.
IIRC 5 was chosen because it's the size of a frame pyramid. I think at one point there was at least one comment mentioning that, although I haven't touched this code in a while so my memory is fuzzy. +1 for making sure we still have a comment mentioning 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.
Improved comment
has_scenecut: delta >= threshold, | ||
} | ||
) -> ScenecutData { | ||
let frame2_ref2 = Arc::clone(&frame2); |
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.
Is there frame2_ref1 or frame2_ref ?
It maybe slightly confusing if we have 2 at the end if there is no frame2_ref or frame2_ref1. Could simply keep ref itself,
IIRC it was there earlier, itself, maybe @lu-zero could say if it is good idea to keep frame2_ref itself or not.
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.
I don't know why it's done the way it is, I just refactored the code)
aa10a8c
to
3097ce2
Compare
3097ce2
to
9594180
Compare
9594180
to
9354fef
Compare
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.
Further to add here to the PR from the #daala IRC discussion,
On checking the BD-Rate1, there seems regression of 8-14% BD-Rates on Speed-10, and also 1 frame displacement in the new scene-change mechanism2, on lower speed levels tested, the BD-Rate remains neutral, while giving a boost in encoding/decoding time generally.
The subsequent frames in the decoded output have +1 change throughout, this happens in the case of a scene-change.
Things to try out,
- Examine the Output of new scene-change with different decoders, and extracting frame-by-frame(maybe with ffmpeg), and see the frames which shown the issue.
- I did not check the code properly to find the +1 change of frame, it is happening to some clips which have scene-change at some levels, need to test at different levels/disabling some encoding features and seeing if it is an offset of some other encoding function,
183e56a
to
2745c52
Compare
@vibhoothi I changed fast scenecut threshold back to what it is on master, can you rerun awcy s10? |
2745c52
to
4fca2a0
Compare
Please rebase the whole thing. |
44bfce4
to
b4d5b01
Compare
b4d5b01
to
ecfd502
Compare
AWCY vimeo 10s corpusspeed 6
speed 9
speed 10
speed 10 with default scene detection and new algo
|
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.
It is fine for me if it is fine for @shssoichiro .
It is good for me as well |
Requested changes were made; we got an updated AWCY run as requested via IRC
Current fast scene detection in rav1e relatively slow to compared methods of scene detection, and have proclivity to show false results or don't detect scene changes where they appear This pr reworks fast scene detection algorithm, making it faster, better, and more accurate Achieved goals are: Faster decision making ( Both less and more efficient computations ) More accurate scene detection, by adjusting threshold based on previous frames Frame downscale for faster decisions.
Goals and motivation
Current fast scene detection in rav1e relatively slow to compared methods of scene detection, and have proclivity to show false results or don't detect scene changes where they appear
This pr reworks fast scene detection algorithm, making it faster, better, and more accurate
Achieved goals are:
Example of adaptive threshold not cutting high motion segment which default static threshold method would cut
Speed comparison
2160p
720p