Skip to content
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.

Sign up for GitHub

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

Use modern triple equals and change to js module #59

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use modern triple equals and change to js module #59

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af22f41
replace all comparison operators with modern triple equals
Nikaoto Mar 3, 2018
d228817
change to js module - usable with webpack and all that good stuff
Nikaoto Mar 3, 2018
fb7948f
Synced original repo
Nikaoto Oct 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions whammy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
vid.compile()
*/

window.Whammy = (function(){
const Whammy = (function(){
// in this case, frames has a very specific meaning, which will be
// detailed once i finish writing the code

Expand Down Expand Up @@ -217,7 +217,7 @@ window.Whammy = (function(){
}
var data = generateEBML([segment.data[i]], outputAsArray);
position += data.size || data.byteLength || data.length;
if (i != 2) { // not cues
if (i !== 2) { // not cues
//Save results to avoid having to encode everything twice
segment.data[i] = data;
}
Expand All @@ -233,8 +233,8 @@ window.Whammy = (function(){
height = frames[0].height,
duration = frames[0].duration;
for(var i = 1; i < frames.length; i++){
if(frames[i].width != width) throw "Frame " + (i + 1) + " has a different width";
if(frames[i].height != height) throw "Frame " + (i + 1) + " has a different height";
if(frames[i].width !== width) throw "Frame " + (i + 1) + " has a different width";
if(frames[i].height !== height) throw "Frame " + (i + 1) + " has a different height";
if(frames[i].duration < 0 || frames[i].duration > 0x7fff) throw "Frame " + (i + 1) + " has a weird duration (must be between 0 and 32767)";
duration += frames[i].duration;
}
Expand Down Expand Up @@ -303,9 +303,9 @@ window.Whammy = (function(){
}

var data = json[i].data;
if(typeof data == 'object') data = generateEBML(data, outputAsArray);
if(typeof data == 'number') data = ('size' in json[i]) ? numToFixedBuffer(data, json[i].size) : bitsToBuffer(data.toString(2));
if(typeof data == 'string') data = strToBuffer(data);
if(typeof data === 'object') data = generateEBML(data, outputAsArray);
if(typeof data === 'number') data = ('size' in json[i]) ? numToFixedBuffer(data, json[i].size) : bitsToBuffer(data.toString(2));
if(typeof data === 'string') data = strToBuffer(data);

if(data.length){
var z = z;
Expand Down Expand Up @@ -339,11 +339,11 @@ window.Whammy = (function(){
}

function toFlatArray(arr, outBuffer){
if(outBuffer == null){
if(outBuffer === null){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I happened to read through this and believe this change introduces a subtle bug. A few lines above in L334, toFlatArray() is called, but without a second argument, i.e. outBuffer will be undefined here. The expected behavior before was that in this case, a new array will be created, and indeed undefined == null in JavaScript (the way it was before your change). Now the test is strict and fails therefore. As a consequence no array is created when outBuffer is undefined, which leads to an error. Maybe it would be best to just test if (!outBuffer) { or if (outBuffer === undefined) {?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a correct remark. I should've noticed that when changing it.

In this case, however, outBuffer is never really undefined since toFlatArray is only used once on L334 outside the function:

// Line 330
		//output as blob or byteArray
		if(outputAsArray){
			//convert ebml to an array
			var buffer = toFlatArray(ebml)
			return new Uint8Array(buffer);
		}else{
			return new Blob(ebml, {type: "video/webm"});
		}

And the variable ebml, defined on L297 can't become undefined since the only changes made to it are a few push methods. So, in this case, my changes don't introduce fatal bugs, but they're still dangerous considering future modifications made to the code might have an undefined array be passed to toFlatArray.

What's more, there's a better way to export whammy as a module for nodejs without having the browser give an annoying error for accessing a non-existent global module.exports.

I'll fix both issues as soon as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR actually fixes the second issue I wrote about: #46

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the variable ebml, defined on L297 can't become undefined since the only changes made to it are a few push methods

While it is true that the variable embl can't become undefiend, it is actually the second parameter to toFloatArray() (the parameter outBuffer) that I worry about. If toFloatArray() is called in L334 without this second argument, only embl is passed in, which in turn is available as argument arr within toFloatArray(). The second parameter (outBuffer) is undefined in this situation and this results in an error if the arr contents are non-objects and get pushed to outBuffer.

The other changes look good to me though and the PR itself is certainly useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have slipped my mind. outBuffer is definitely undefined when no second argument is passed to toFloatArray.

outBuffer = [];
}
for(var i = 0; i < arr.length; i++){
if(typeof arr[i] == 'object'){
if(typeof arr[i] === 'object'){
//an array
toFlatArray(arr[i], outBuffer)
}else{
Expand Down Expand Up @@ -375,8 +375,8 @@ window.Whammy = (function(){
var ebml = '';
for(var i = 0; i < json.length; i++){
var data = json[i].data;
if(typeof data == 'object') data = generateEBML_old(data);
if(typeof data == 'number') data = toBinStr_old(data.toString(2));
if(typeof data === 'object') data = generateEBML_old(data);
if(typeof data === 'number') data = toBinStr_old(data.toString(2));

var len = data.length;
var zeroes = Math.ceil(Math.ceil(Math.log(len)/Math.log(2))/8);
Expand Down Expand Up @@ -449,15 +449,15 @@ window.Whammy = (function(){
while (offset < string.length) {
var id = string.substr(offset, 4);
chunks[id] = chunks[id] || [];
if (id == 'RIFF' || id == 'LIST') {
if (id === 'RIFF' || id === 'LIST') {
var len = parseInt(string.substr(offset + 4, 4).split('').map(function(i){
var unpadded = i.charCodeAt(0).toString(2);
return (new Array(8 - unpadded.length + 1)).join('0') + unpadded
}).join(''),2);
var data = string.substr(offset + 4 + 4, len);
offset += 4 + 4 + len;
chunks[id].push(parseRIFF(data));
} else if (id == 'WEBP') {
} else if (id === 'WEBP') {
// Use (offset + 8) to skip past "VP8 "/"VP8L"/"VP8X" field after "WEBP"
chunks[id].push(string.substr(offset + 8));
offset = string.length;
Expand Down Expand Up @@ -487,23 +487,23 @@ window.Whammy = (function(){
.join('') // join the bytes in holy matrimony as a string
}

function WhammyVideo(speed, quality){ // a more abstract-ish API
function WhammyVideo(speed, quality) { // a more abstract-ish API
this.frames = [];
this.duration = 1000 / speed;
this.quality = quality || 0.8;
}

WhammyVideo.prototype.add = function(frame, duration){
if(typeof duration != 'undefined' && this.duration) throw "you can't pass a duration if the fps is set";
if(typeof duration == 'undefined' && !this.duration) throw "if you don't have the fps set, you need to have durations here.";
if(typeof duration !== 'undefined' && this.duration) throw "you can't pass a duration if the fps is set";
if(typeof duration === 'undefined' && !this.duration) throw "if you don't have the fps set, you need to have durations here.";
if(frame.canvas){ //CanvasRenderingContext2D
frame = frame.canvas;
}
if(frame.toDataURL){
// frame = frame.toDataURL('image/webp', this.quality);
// quickly store image data so we don't block cpu. encode in compile method.
frame = frame.getContext('2d').getImageData(0, 0, frame.width, frame.height);
}else if(typeof frame != "string"){
}else if(typeof frame !== "string"){
throw "frame must be a a HTMLCanvasElement, a CanvasRenderingContext2D or a DataURI formatted string"
}
if (typeof frame === "string" && !(/^data:image\/webp;base64,/ig).test(frame)) {
Expand All @@ -517,7 +517,6 @@ window.Whammy = (function(){

// deferred webp encoding. Draws image data to canvas, then encodes as dataUrl
WhammyVideo.prototype.encodeFrames = function(callback){

if(this.frames[0].image instanceof ImageData){

var frames = this.frames;
Expand Down Expand Up @@ -571,3 +570,5 @@ window.Whammy = (function(){
// expose methods of madness
}
})()

module.exports = Whammy