-
Notifications
You must be signed in to change notification settings - Fork 10
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
New functionality #3
Conversation
Pull Request Test Coverage Report for Build 11
💛 - Coveralls |
Pull Request Test Coverage Report for Build 17
💛 - Coveralls |
You use AR_CALCULATION_PRECISION to normalize result of division but you don't have tests for that. |
README.md
Outdated
`areAllGopsIdentical` field. | ||
For the full GOPs `processFrames` calculates min/max/mean values of bitrates (in kbit/s), framerates and GOP duration | ||
(in seconds) and returns them in `payload` field. The result of the check for the similarity of GOP structures for | ||
the collected GOPs is returned in `areAllGopsIdentical` field. Fields `width`, `height` and `aspectRatio` adjust on |
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's better to write Fields `width`, `height` and `aspectRatio` are taken from"
.
and
reflects a presence
-> reflects presence
.
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.
+
README.md
Outdated
mean: 2, | ||
min: 1.9, | ||
max: 2.1 }, | ||
aspectRatio: '16:9', |
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's better to describe how aspectRatio
calculates.
src/processFrames.js
Outdated
function processFrames(frames) { | ||
if (!Array.isArray(frames)) { | ||
throw new TypeError('process method is supposed to accept an array of frames.'); | ||
} | ||
|
||
const audioFrames = processFrames.filterAudioFrames(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.
You don't use audioFrames
array, so you may replace filterAudioFrames
function with smth like hasAudioFrames
and use Array.some
method. It's much more efficient.
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.
good remark
src/processFrames.js
Outdated
return WIDESCREEN_AR; | ||
} | ||
|
||
return `${width}:${height}`; |
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 think that we need to calculate GCD. It's common technique in ffmpeg.
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.
+
src/processFrames.js
Outdated
const baseGopSize = gops[0].frames.length; | ||
const bitrates = []; | ||
const fpsList = []; | ||
const gopsDurations = []; |
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.
Btw, you can't use gopsDurations. Choose one of the following options: gopDurations or gopsDuration.
In my opinion, gopDurations 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.
+ gopDurations
video-quality-tools v1.1.0
processFrames:
Added new fields
gopDuration
,displayAspectRatio
,width
,height
,hasAudioStream
to the result of processFrames execution .Add new methods to processFrames:
calculateGopDuration
,calculateDisplayAspectRatio
,hasAudioFrames
.FramesMonitor
FramesMonitor fetches video and audio frames from the stream now.
Added
width
andheight
info to video frames.