Support video source #48

jongmoon · 2017-11-13T07:58:01Z

Issue

Details

We need to discuss video interface on this branch (video#44)

image.complete is not valid to check image is loaded. Ref: https://stackoverflow.com/questions/23657424/why-image-complete-property-always-return-true-even-if-there-is-no-src-tag Ref naver#44

Ref naver#44

coveralls · 2017-11-13T08:06:51Z

Coverage increased (+13.9%) to 51.374% when pulling d71b053 on jongmoon:video#44 into 98f6af5 on naver:video#44.

Ref naver#49

Apply axes 2.3.0

jongmoon · 2017-11-16T06:01:07Z

src/PanoImageRenderer/PanoImageRenderer.js

@@ -38,7 +39,7 @@ const ERROR_TYPE = {
 };

 export default class PanoImageRenderer extends Component {
-	constructor(image, width, height, sphericalConfig) {


isVideo is used to distinguish image is image or video.

Other consideration

image => content

Issue: It's difficult to distinguish image from image if content is string.

Add isVideo property on sphericalConfig

Issue: isVideo is not related with sphericalConfig

Understood.

jongmoon · 2017-11-16T06:02:40Z

src/PanoImageRenderer/PanoImageRenderer.js


-		this._onImageLoad = 	this._onImageLoad.bind(this);
-		this._onImageError = 	this._onImageError.bind(this);


I've replaced image with content.

media was a candidate.

Because the media element only contains video and audio, the name media is not appropriate for image and video.

I'm okay with unified loader name as content loader.

jongmoon · 2017-11-16T06:04:55Z

src/PanoImageRenderer/PanoImageRenderer.js

 	}

-	setImage({image, imageType}) {


I don't separate a function into video and image. because their logic are almost same.

jongmoon · 2017-11-16T06:08:28Z

src/PanoImageRenderer/PanoImageRenderer.js

+			this._contentLoader = new VideoLoader();
+		} else {
+			this._contentLoader = new ImageLoader();
+		}


contentLoader uses concept of inheritance. and use get/set common interface.

I've tried to combine imageLoader and videoLoader. But it is somewhat different logic. If being combined, code will be difficult to read.

I agree with this.

jongmoon · 2017-11-16T06:09:08Z

src/PanoImageRenderer/PanoImageRenderer.js

@@ -145,19 +156,20 @@ export default class PanoImageRenderer extends Component {
 	}

 	cancelLoadImage() {
-		this._imageLoader.cancelLoadImage();
+		this._contentLoader.destroy();


I removed cancelImage. and replaced it destroy.

jongmoon · 2017-11-16T06:11:02Z

src/PanoImageRenderer/PanoImageRenderer.js

+			this._contentLoader.get()
+				.then(() => this._bindTexture())
+				.then(res)
+				.catch(rej);


Use catch to check res fails.

The line with catch cannot resolve failing situation.

inst.bindTexture().then(() => {}).catch(() => {});

jongmoon · 2017-11-16T06:15:27Z

src/PanoViewer/PanoViewer.js

@@ -32,7 +33,7 @@ export default class PanoViewer extends Component {
 	 * @param {Array} [config.fovRange=[30, 110]] Range of controllable vertical field of view values <ko>제어 가능한 수직 field of view 값의 범위</ko>
 	 * @param {Function} [config.checkSupport] A function that returns a boolean value that determines whether the component is working. <ko>뷰어가 작동할 지 여부를 결정하는 부울 값을 반환하는 함수입니다.</ko>
 	 */
-	constructor(container, options) {
+	constructor(container, options = {}) {


Script error will be prevented if options are omitted.

jongmoon · 2017-11-16T07:47:39Z

src/PanoViewer/PanoViewer.js

@@ -57,8 +58,19 @@ export default class PanoViewer extends Component {
 			return this;
 		}

+		if (!!options.image === !!options.video) {


If image and video are both omitted or both specified, then trigger INVALID_RESOURCE error. In other words, only one resource (image or video) can be specified.

Great.

There should be documentation for the new event.
like below

/** * Events that is fired when animation which is triggered by inertia is ended. * @ko 관성에 의한 애니메이션 동작이 완료되었을때 발생하는 이벤트 * @name eg.view360.PanoViewer#animationEnd * @event * @example * * viwer.on({ * "animationEnd" : function(evt) { * // animation is ended. * }); */

this jsdoc is written inside of _triggerEvent method.

OK, I will apply it on PanoViewer#error

jongmoon · 2017-11-16T08:09:17Z

src/PanoViewer/PanoViewer.js

+		return {
+			video: this._image,
+			type: this._imageType
+		};


Be careful its return type is differ from getImage();

getVideo returns,

video:

type: projectionType (equirectangular or cubemap)

getImage returns

image: Image Object

imageType: projectionType (equirectangular or cubemap)

Both return projection type but it expose other interface(type/imageType) each other.

@happyhj As you mentioned in offline, I think it would be better not to expose type information.

I found imageType accessing code that use like below.

panoViewer.setImage({ image: image, imageType: panoViewer.getImage().imageType });

We need to decide name of imageType getter.

projectionType

imageType

type

and others?

happyhj · 2017-11-16T08:23:59Z

src/PanoImageRenderer/ImageLoader.js

 		}

 		// promise for image
 		return this.get();
 	}

+	static _isMaybeLoaded(image) {
+		return image && (image.width || image.height);


width and height property in image object is not read-only.
However, naturalWidth is read-only, so it is good to check that the image has been loaded.

The complete property is not appropriate because image objects that do not have src will also have true values.

I suggest that you apply the following statement.

return image && (image.naturalWidth !== 0);

@happyhj good information, thank you!

happyhj · 2017-11-16T09:00:05Z

src/PanoImageRenderer/PanoImageRenderer.js


-		this._onImageLoad = 	this._onImageLoad.bind(this);
-		this._onImageError = 	this._onImageError.bind(this);


Because the media element only contains video and audio, the name media is not appropriate for image and video.

I'm okay with unified loader name as content loader.

happyhj · 2017-11-16T09:02:06Z

src/PanoImageRenderer/PanoImageRenderer.js

+			this._contentLoader = new VideoLoader();
+		} else {
+			this._contentLoader = new ImageLoader();
+		}


I agree with this.

happyhj · 2017-11-16T09:02:18Z

src/PanoImageRenderer/PanoImageRenderer.js

 	}

-	setImage({image, imageType}) {


happyhj · 2017-11-16T09:17:26Z

src/PanoImageRenderer/PanoImageRenderer.js

@@ -378,7 +390,7 @@ export default class PanoImageRenderer extends Component {
 	}

 	renderWithQuaternion(quaternion, fieldOfView) {
-		if (!this.hasRenderingContext()) {
+		if (!this.isImageLoaded() || !this.hasRenderingContext()) {


The isImageLoaded method seems like no longer belongs to the PanoImageRenderer.

How about removing isImageLoaded method from PanoImageRenderer and checking loading status with method on VideoLoader and ImageLoader?

like

this._imageLoader.isContentReady(); this._videoLoader.isContentReady();

@happyhj I got it!

happyhj · 2017-11-16T09:18:45Z

src/PanoViewer/PanoViewer.js

@@ -32,7 +33,7 @@ export default class PanoViewer extends Component {
 	 * @param {Array} [config.fovRange=[30, 110]] Range of controllable vertical field of view values <ko>제어 가능한 수직 field of view 값의 범위</ko>
 	 * @param {Function} [config.checkSupport] A function that returns a boolean value that determines whether the component is working. <ko>뷰어가 작동할 지 여부를 결정하는 부울 값을 반환하는 함수입니다.</ko>
 	 */
-	constructor(container, options) {
+	constructor(container, options = {}) {


happyhj · 2017-11-16T09:19:06Z

src/PanoViewer/PanoViewer.js

@@ -57,8 +58,19 @@ export default class PanoViewer extends Component {
 			return this;
 		}

+		if (!!options.image === !!options.video) {


Great.

There should be documentation for the new event.
like below

/** * Events that is fired when animation which is triggered by inertia is ended. * @ko 관성에 의한 애니메이션 동작이 완료되었을때 발생하는 이벤트 * @name eg.view360.PanoViewer#animationEnd * @event * @example * * viwer.on({ * "animationEnd" : function(evt) { * // animation is ended. * }); */

this jsdoc is written inside of _triggerEvent method.

…o video#44

coveralls · 2017-11-29T11:09:57Z

Coverage increased (+16.9%) to 54.342% when pulling ba83d18 on jongmoon:video#44 into 98f6af5 on naver:video#44.

jongmoon · 2017-11-30T01:59:23Z

src/PanoImageRenderer/VideoLoader.js

+export default class VideoLoader {
+	constructor(video) {
+		this._isLoaded = false;
+		this._handlers = [];


@happyhj I added event handler for managing content load event. Because if not managed, it fires event although it has been destroyed.

jongmoon · 2017-11-30T02:01:20Z

src/PanoViewer/PanoViewer.js

@@ -57,8 +58,19 @@ export default class PanoViewer extends Component {
 			return this;
 		}

+		if (!!options.image && !!options.video) {


@happyhj I had changed above condition if (!!options.image === !!options.video). but I changed it . because of allowing no content specified.

jongmoon · 2017-11-30T02:03:07Z

src/PanoViewer/PanoViewer.js

+		}
+
+		return this._photoSphereRenderer.getContent();
+	}


@happyhj getVideo returns HTMLVideoElement

Ref naver#44

coveralls · 2017-11-30T02:12:08Z

Coverage increased (+16.9%) to 54.301% when pulling 80bd42c on jongmoon:video#44 into 98f6af5 on naver:video#44.

jongmoon · 2017-11-30T02:37:53Z

src/PanoImageRenderer/ImageLoader.js

+		 * Init event handlers
+		 */
+		this._image.onload = null;
+		this._image.onerror = null;


@happyhj Initialize event handler for preventing event from being fired after destroy

need to remove listeners which attached with addEventListener, just like in VideoLoader.js

jongmoon · 2017-11-30T02:43:50Z

src/PanoImageRenderer/PanoImageRenderer.js

+			content: this._image,
+			isVideo: this._isVideo,
+			projectionType: this._imageType
+		});


@happyhj Renderer triggers IMAGE_LOADED event with param {content, isVideo, projectionType}

Ref naver#44

jongmoon · 2017-11-30T02:51:01Z

src/PanoViewer/PanoViewer.js

-		this._imageType = options.imageType || PanoImageRenderer.ImageType.EQUIRECTANGULAR;
+		this._image = options.image || options.video;
+		this._isVideo = !!options.video;
+		this._projectionType = options.projectionType || PanoImageRenderer.ImageType.EQUIRECTANGULAR;


@happyhj _imageType -> _projectionType on PanoViewer. It is not applied on PanoImageRenderer

jongmoon · 2017-11-30T02:56:57Z

src/PanoViewer/PanoViewer.js

+		if (this._image && isVideo !== this._isVideo) {
+			console.warn("Not support changing content type(Image <--> Video)");
+			return this;
+		}


@happyhj I'm begging your opinion for this.

In above code, if content type is differed then it will do nothing. I consider following way if set fails.

throw

return false (if fails)

console

ignore

But each have each problem.

User should try/catch

Different set interface (others returns this)

User may not recognize it.

User cannot judge what is the problem.

console.warn would be nice.
And it will be better to show this is temporary state with warning message.

"Currently not supporting changing content type(Image <--> Video)"

jongmoon · 2017-11-30T02:57:32Z

src/PanoViewer/PanoViewer.js

 			}
 		);
 		this._bindRendererHandler();
 	}

 	_bindRendererHandler() {
+		this._photoSphereRenderer.on(PanoImageRenderer.EVENTS.IMAGE_LOADED, e => {
+			this.trigger(EVENTS.CONTENT_LOADED, e);


@happyhj I added "contentLoaded" event.

Ref naver#44

happyhj · 2017-11-30T09:11:00Z

src/PanoImageRenderer/PanoImageRenderer.js

+			this._contentLoader.get()
+				.then(() => this._bindTexture())
+				.then(res)
+				.catch(rej);


The line with catch cannot resolve failing situation.

inst.bindTexture().then(() => {}).catch(() => {});

happyhj · 2017-11-30T09:21:40Z

src/PanoImageRenderer/ImageLoader.js

+				res(this._image);
+			} else if (this._loadStatus === STATUS.LOADING) {
+				ImageLoader._once(this._image, "load", () => {
+					this._loadStatus = STATUS.LOADED;


we don't need to set state with getter method.

happyhj · 2017-11-30T09:21:47Z

src/PanoImageRenderer/ImageLoader.js

-				ImageLoader._once(this.image, "error", () => {
-					rej("failed to load images.");
+				ImageLoader._once(this._image, "error", () => {
+					this._loadStatus = STATUS.ERROR;


we don't need to set state with getter method.

happyhj · 2017-11-30T09:24:29Z

src/PanoImageRenderer/ImageLoader.js

+		 * Init event handlers
+		 */
+		this._image.onload = null;
+		this._image.onerror = null;


need to remove listeners which attached with addEventListener, just like in VideoLoader.js

happyhj · 2017-11-30T09:31:42Z

src/PanoImageRenderer/VideoLoader.js

+				res(this._video);
+			} else {
+				this._once("canplaythrough", () => {
+					this._isLoaded = false;


No need to set loading status in setter method.

happyhj · 2017-11-30T09:33:16Z

src/PanoImageRenderer/VideoLoader.js

+	}
+
+	set(video) {
+		if (typeof video === "string") {


Setting loading status is missing. like

this._once("canplaythrough", () => { this._isLoaded = true; });

and should check the case that the video element already triggered canplaythrough event.
in that case canplaythrough event will not triggered.

@happyhj So I check it video status like below on getter.

else if (this._video.readyState === READY_STATUS.HAVE_ENOUGH_DATA) { res(this._video); }

happyhj · 2017-11-30T09:39:52Z

src/PanoImageRenderer/renderer/SphereRenderer.js

-		const width = image.width;// imageWidth;
-		const height = image.height;// imageHeight;
+		const width = image.width || image.videoWidth;// imageWidth;
+		const height = image.height || image.videoHeight;// imageHeight;


It is safer to use read-only property like videoWidth of video element.
How about using naturalWidth and naturalHeight property for image element?

happyhj · 2017-11-30T10:10:30Z

src/PanoViewer/PanoViewer.js

+		if (this._image && isVideo !== this._isVideo) {
+			console.warn("Not support changing content type(Image <--> Video)");
+			return this;
+		}


console.warn would be nice.
And it will be better to show this is temporary state with warning message.

"Currently not supporting changing content type(Image <--> Video)"

- refactoring ImageLoader

use readyState to check if loaded Ref naver#44

add getter for loadStatus Ref naver#44

change log Ref naver#44

Ref naver#44

replace width/height with naturalWidth/naturalHeight to use origingal size Ref naver#44

coveralls · 2017-12-01T06:02:52Z

Coverage increased (+16.8%) to 54.206% when pulling b7930d1 on jongmoon:video#44 into 98f6af5 on naver:video#44.

happyhj

Looks great!
And I also reviewed test code.

happyhj · 2017-12-01T03:51:15Z

test/unit/PanoImageRenderer/ImageLoader.spec.js

-			this.inst = new ImageLoader("https://s.pstatic.net/static/www/mobile/edit/2015/0318/mobile_110629401121.png");
+		it("should get image URL as a parameter", function(done) {
+			// this.inst = new ImageLoader("https://s.pstatic.net/static/www/mobile/edit/2015/0318/mobile_110629401121.png");
+			this.inst = new ImageLoader("./images/PanoViewer/waterpark_preview.jpg");


It is better to use naturalWidth/Height property rather than width/height for checking image is fully loaded.

assert.isOk(image); assert.isOk(image.complete); assert.isNumber(image.naturalWidth); assert.isNumber(image.naturalHeight); assert.notEqual(image.naturalWidth, 0); // need to check dimension is not 0 assert.notEqual(image.naturalHeight, 0); // need to check dimension is not 0

and please add given, when, then annotation.

happyhj · 2017-12-01T04:06:32Z

test/unit/PanoImageRenderer/ImageLoader.spec.js

-				console.log(msg);
-				done();
-			});
+					console.log(msg);


Missing assertion statement.
Calling done method means only the test is ended.

And should not call done method only in the reject callback.
If the reject callback is not called, this test will not end forever.

happyhj · 2017-12-01T04:11:07Z

test/unit/PanoImageRenderer/ImageLoader.spec.js

 			// Given
 			// When
 			this.inst = new ImageLoader();

-			this.inst.setImage("https://s.pstatic.net/static/www/mobile/edit/2015/0318/mobile_110629401121.png").then((img) => {
+			return this.inst.set("./images/PanoViewer/waterpark_preview.jpg").then(img => {
 				assert.isOk(img.complete);


Missing to call done method in asynchronous testing.

happyhj · 2017-12-01T04:14:15Z

test/unit/PanoImageRenderer/ImageLoader.spec.js

+			const inst = new ImageLoader(imgObj); // image is not loaded.
+
+			// Then
+			expect(inst._loadStatus).to.be.equal(1); // 2 is LOADING status


Can we make this case without checking private property?

Because making change to _loadStatus will make this test break.

happyhj · 2017-12-01T04:15:22Z

test/unit/PanoImageRenderer/ImageLoader.spec.js

+				inst = new ImageLoader(imgObj);
+
+				// Then
+				expect(inst._loadStatus).to.be.equal(2); // 2 is LOADED status


Using private property, too.

happyhj · 2017-12-01T05:42:51Z

test/unit/PanoImageRenderer/ImageLoader.spec.js


+		it("should resolve image after image is loaded successfully", () => {
+			// Given


this test cannot guarantee the image object delivered with resolve callback is fully loaded. It guarantees only resolve callback is calling.

How about below?

it("should resolve image after image is loaded successfully", (done) => { // Given // When const inst = new ImageLoader("./images/PanoViewer/waterpark_preview.jpg"); inst.get() .then((image) => { // then const isLoaded = image && image.naturalWidth !== 0; expect(isLoaded, "resolve image after image has been loaded.").to.be.true; done(); }); });

happyhj · 2017-12-01T05:46:26Z

test/unit/PanoImageRenderer/VideoLoader.spec.js

+			expect(this.inst).to.be.exist;
+			this.inst.get().then(null, (msg)=> {
+					console.log(msg);
+					done();


if resolve is not called, test will not end.

happyhj · 2017-12-01T05:48:24Z

test/unit/PanoImageRenderer/VideoLoader.spec.js

+			this.inst.get().then(video => {
+				assert.isOk(video);
+				assert.isTrue(video instanceof HTMLVideoElement);
+				done();


I don't suggest place done method inside of resolve callback.
What do you think about using Promise.prototype.finally?

change aync test can be stopped Ref naver#44

coveralls · 2017-12-01T07:28:42Z

Coverage increased (+16.8%) to 54.206% when pulling c4c0b01 on jongmoon:video#44 into 98f6af5 on naver:video#44.

happyhj · 2017-12-01T07:34:12Z

LGTM

Jongmoon Yoon added 6 commits November 10, 2017 19:05

feat(VideoLoader): add video loader module

0431ca2

Ref naver#44

fix(ImageLoader): apply loadStatus instead of image.complete

5797ebe

image.complete is not valid to check image is loaded. Ref: https://stackoverflow.com/questions/23657424/why-image-complete-property-always-return-true-even-if-there-is-no-src-tag Ref naver#44

feat(all): support video content

ab1fc7e

Ref naver#44

skip: remove only flag

2a93d8c

test(ImageLoader): satisfy 100% coverage

e3c2d90

skip: fix test typo error

d71b053

Jongmoon Yoon added 4 commits November 13, 2017 18:02

chore(all): apply circular fixed axes version.

38d5e0b

Ref naver#49

docs(README): update axes version.

41abc1a

Ref naver#49

test(YawPitchControl): update test case by fixing circular bugs.

d31fac3

Ref naver#49

Merge pull request naver#50 from jongmoon/axes#49

6411324

Apply axes 2.3.0

jongmoon commented Nov 16, 2017

View reviewed changes

happyhj reviewed Nov 16, 2017

View reviewed changes

Jongmoon Yoon added 6 commits November 17, 2017 12:19

Merge branch 'master' of https://github.com/jongmoon/egjs-view360 int…

e3009b1

…o video#44

skip: apply review, jsdoc for error message.

a14813e

skip: review applied. use read-only property to check image loaded.

fb5af1d

fix(ImageLoader): prevent event firing after destroy

bef8a12

skip: apply review - change set/get interface

7b4f6f3

fix(VideoLoader): prevent event firring after destroy

ba83d18

jongmoon commented Nov 30, 2017

View reviewed changes

feat(PanoViewer): replace imageType with projectionType

80bd42c

Ref naver#44

jongmoon commented Nov 30, 2017

View reviewed changes

test(PanoViewer): apply changed interface on demo

d01a7c7

Ref naver#44

jongmoon commented Nov 30, 2017

View reviewed changes

test(PanoViewer): PanoVideo -resize handling

b0ac8f1

Ref naver#44

happyhj suggested changes Nov 30, 2017

View reviewed changes

Jongmoon Yoon added 6 commits December 1, 2017 11:23

skip: review apply

2a9a418

- refactoring ImageLoader

skip: review apply

61e3e81

use readyState to check if loaded Ref naver#44

skip: review applied

1b6d7cf

add getter for loadStatus Ref naver#44

skip: review applied

6710f4b

change log Ref naver#44

skip: review applied

de59b6d

Ref naver#44

skip: review apply

b7930d1

replace width/height with naturalWidth/naturalHeight to use origingal size Ref naver#44

happyhj approved these changes Dec 1, 2017

View reviewed changes

skip: apply review

c4c0b01

change aync test can be stopped Ref naver#44

jongmoon merged commit 3111655 into naver:video#44 Dec 1, 2017

jongmoon mentioned this pull request Mar 19, 2018

Update README for 3.0.0 #174

Closed


		this._onImageLoad = this._onImageLoad.bind(this);
		this._onImageError = this._onImageError.bind(this);


		it("should resolve image after image is loaded successfully", () => {
		// Given

Support video source #48

Support video source #48

Conversation

jongmoon commented Nov 13, 2017

Issue

Details

coveralls commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongmoon Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2017

happyhj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2017

happyhj commented Dec 1, 2017

jongmoon Nov 16, 2017 •

edited

Loading

happyhj left a comment •

edited

Loading