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

Proposed solution for issue #676 "Chapters are broken in 4.x" #1221

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Chris-DL
Contributor

Chris-DL commented May 20, 2014

Proposed solution for issue #676 "Chapters are broken in 4.x"

Added Chapters Button style in css (defaulted with speech bubble icon).

Added functionality to load chapter tracks and add menu control to player.

I welcome any feedback on the solution.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 20, 2014

Member

Nice, thanks! I'll dig into it soon.

Member

heff commented May 20, 2014

Nice, thanks! I'll dig into it soon.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 20, 2014

Member

Do you have a good example chapters webvtt file that can be used to test this with?

Member

heff commented May 20, 2014

Do you have a good example chapters webvtt file that can be used to test this with?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 21, 2014

Member

I tried testing this just using the sandbox/index.html file and adding a chapters track, and I can't get the chapters control to show itself.

I can force it to show by doing player.controlBar.chaptersButton.show(), but the chapters list is empty when I do that.

And if I check player.textTracks_ after the player is ready, the text track object is there.

So for some reason it's not getting the chapters to the button. Any ideas?

Member

heff commented May 21, 2014

I tried testing this just using the sandbox/index.html file and adding a chapters track, and I can't get the chapters control to show itself.

I can force it to show by doing player.controlBar.chaptersButton.show(), but the chapters list is empty when I do that.

And if I check player.textTracks_ after the player is ready, the text track object is there.

So for some reason it's not getting the chapters to the button. Any ideas?

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 21, 2014

Contributor

Hi @heff
Here is an example vtt file for the chapters (taken from the existing demo.captions.vtt)

WEBVTT

Chapter 1
00:00.700 --> 00:04.110
[ Heroic music playing for a seagull ]

Chapter 2
00:04.500 --> 00:05.000
[ Splash!!! ]

Chapter 3
00:05.100 --> 00:06.000
[ Sploosh!!! ]

Chapter 4
00:08.000 --> 00:09.225
[ Splash...splash...splash splash splash ]

Chapter 5
00:10.525 --> 00:11.255
[ Splash, Sploosh again ]

Chapter 6
00:13.500 --> 00:14.984
Dolphin: eeeEEEEEeeee!

Chapter 7
00:14.984 --> 00:16.984
Dolphin: Squawk! eeeEEE?

Chapter 8
00:25.000 --> 00:28.284
[ A whole ton of splashes ]

Chapter 9
00:29.500 --> 00:31.000
Mine. Mine. Mine.

Chapter 10
00:34.300 --> 00:36.000
Shark: Chomp

Chapter 11
00:36.800 --> 00:37.900
Shark: CHOMP!!!

Chapter 12
00:37.861 --> 00:41.193
EEEEEEOOOOOOOOOOWHALENOISE

Chapter 13
00:42.593 --> 00:45.611
[ BIG SPLASH ]

Contributor

Chris-DL commented May 21, 2014

Hi @heff
Here is an example vtt file for the chapters (taken from the existing demo.captions.vtt)

WEBVTT

Chapter 1
00:00.700 --> 00:04.110
[ Heroic music playing for a seagull ]

Chapter 2
00:04.500 --> 00:05.000
[ Splash!!! ]

Chapter 3
00:05.100 --> 00:06.000
[ Sploosh!!! ]

Chapter 4
00:08.000 --> 00:09.225
[ Splash...splash...splash splash splash ]

Chapter 5
00:10.525 --> 00:11.255
[ Splash, Sploosh again ]

Chapter 6
00:13.500 --> 00:14.984
Dolphin: eeeEEEEEeeee!

Chapter 7
00:14.984 --> 00:16.984
Dolphin: Squawk! eeeEEE?

Chapter 8
00:25.000 --> 00:28.284
[ A whole ton of splashes ]

Chapter 9
00:29.500 --> 00:31.000
Mine. Mine. Mine.

Chapter 10
00:34.300 --> 00:36.000
Shark: Chomp

Chapter 11
00:36.800 --> 00:37.900
Shark: CHOMP!!!

Chapter 12
00:37.861 --> 00:41.193
EEEEEEOOOOOOOOOOWHALENOISE

Chapter 13
00:42.593 --> 00:45.611
[ BIG SPLASH ]

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 21, 2014

Member

If you load that as a track in sandbox/index.html in this branch, does it work for you?

Member

heff commented May 21, 2014

If you load that as a track in sandbox/index.html in this branch, does it work for you?

@heff heff added the enhancement label May 21, 2014

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 21, 2014

Contributor

@heff

Yes, the chapters do load, but maybe the solution needs a slight tweak to not expect a "default" value for the track.

Here is what I did:

  1. Create a new vtt file named "demo.chapters.vtt" with the text above and place that file into /build/demo-files/
  2. In sandbox/index.html replace line 27 with:
 <track kind="chapters" src="../build/demo-files/demo.chapters.vtt" srclang="en" label="English" default></track>
  1. Load up the index.html and the chapters should display.

Hopefully this helps. Please let me know if you have any other questions and comments.

Contributor

Chris-DL commented May 21, 2014

@heff

Yes, the chapters do load, but maybe the solution needs a slight tweak to not expect a "default" value for the track.

Here is what I did:

  1. Create a new vtt file named "demo.chapters.vtt" with the text above and place that file into /build/demo-files/
  2. In sandbox/index.html replace line 27 with:
 <track kind="chapters" src="../build/demo-files/demo.chapters.vtt" srclang="en" label="English" default></track>
  1. Load up the index.html and the chapters should display.

Hopefully this helps. Please let me know if you have any other questions and comments.

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 21, 2014

Contributor

@heff Sorry, looks like my code was removed for the index.html.

<track kind="chapters" src="../build/demo-files/demo.chapters.vtt" srclang="en" label="English" default></track>
Contributor

Chris-DL commented May 21, 2014

@heff Sorry, looks like my code was removed for the index.html.

<track kind="chapters" src="../build/demo-files/demo.chapters.vtt" srclang="en" label="English" default></track>
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 21, 2014

Member

Ah, yeah, the default is what I was missing. I'm not sure if default should be required to show the chapters menu or not. None of the browsers I'm trying seem to support chapters natively, so can't tell from that, but my guess is that it shouldn't require a default. Also none of the other track menus require a default to show the menu.

So far this is looking great. I'm seeing one weird double title issue, where I can see "chapters" twice in the menu. Are you seeing this?

screen shot 2014-05-21 at 3 57 21 pm

Member

heff commented May 21, 2014

Ah, yeah, the default is what I was missing. I'm not sure if default should be required to show the chapters menu or not. None of the browsers I'm trying seem to support chapters natively, so can't tell from that, but my guess is that it shouldn't require a default. Also none of the other track menus require a default to show the menu.

So far this is looking great. I'm seeing one weird double title issue, where I can see "chapters" twice in the menu. Are you seeing this?

screen shot 2014-05-21 at 3 57 21 pm

@rcrooks

This comment has been minimized.

Show comment
Hide comment
@rcrooks

rcrooks May 22, 2014

Contributor

I like this(!), but might be better if the list could show chapter titles (if any) instead of just numbers.

Robert
-- 
Robert Crooks
Director of Learning Services

From: Steve Heffernan notifications@github.com
Reply: videojs/video.js reply@reply.github.com
Date: May 21, 2014 at 7:10:24 PM
To: videojs/video.js video.js@noreply.github.com
Subject:  Re: [video.js] Proposed solution for issue #676 "Chapters are broken in 4.x" (#1221)

Ah, yeah, the default is what I was missing. I'm not sure if default should be required to show the chapters menu or not. None of the browsers I'm trying seem to support chapters natively, so can't tell from that, but my guess is that it shouldn't require a default. Also none of the other track menus require a default to show the menu.

So far this is looking great. I'm seeing one weird double title issue, where I can see "chapters" twice in the menu. Are you seeing this?


Reply to this email directly or view it on GitHub.

Contributor

rcrooks commented May 22, 2014

I like this(!), but might be better if the list could show chapter titles (if any) instead of just numbers.

Robert
-- 
Robert Crooks
Director of Learning Services

From: Steve Heffernan notifications@github.com
Reply: videojs/video.js reply@reply.github.com
Date: May 21, 2014 at 7:10:24 PM
To: videojs/video.js video.js@noreply.github.com
Subject:  Re: [video.js] Proposed solution for issue #676 "Chapters are broken in 4.x" (#1221)

Ah, yeah, the default is what I was missing. I'm not sure if default should be required to show the chapters menu or not. None of the browsers I'm trying seem to support chapters natively, so can't tell from that, but my guess is that it shouldn't require a default. Also none of the other track menus require a default to show the menu.

So far this is looking great. I'm seeing one weird double title issue, where I can see "chapters" twice in the menu. Are you seeing this?


Reply to this email directly or view it on GitHub.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 22, 2014

Member

Actually that's a good point. The demo file we're using is really captions, so not the great at showing a real use case. We should try the one from this example.
http://www.html5videoguide.net/demos/google_io/3_navigation/
direct file link
http://www.html5videoguide.net/demos/google_io/3_navigation/webvtt_talk_navigation.vtt

In both that example, and this one from html5 doctor it would seem the best approach is to show the cue ID plus the cue text. e.g. if ID is "Chapter 1" and text is "Introduction", the menu item should read "Chapter 1: Introduction".

It might be good to find other chapter file examples/recommendations as well.

Member

heff commented May 22, 2014

Actually that's a good point. The demo file we're using is really captions, so not the great at showing a real use case. We should try the one from this example.
http://www.html5videoguide.net/demos/google_io/3_navigation/
direct file link
http://www.html5videoguide.net/demos/google_io/3_navigation/webvtt_talk_navigation.vtt

In both that example, and this one from html5 doctor it would seem the best approach is to show the cue ID plus the cue text. e.g. if ID is "Chapter 1" and text is "Introduction", the menu item should read "Chapter 1: Introduction".

It might be good to find other chapter file examples/recommendations as well.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 22, 2014

Member

@silviapfeiffer, is it pretty standard that for a chapters vtt file, you should combine the cue ID and the cue text to create the link text for a chapter, e.g. "Slide 1: Title Slide" in your example?

Member

heff commented May 22, 2014

@silviapfeiffer, is it pretty standard that for a chapters vtt file, you should combine the cue ID and the cue text to create the link text for a chapter, e.g. "Slide 1: Title Slide" in your example?

Removed default requirement for chapter track.
Fixed duplicate chapter menu display
@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 22, 2014

Contributor

@heff, I made a couple of changes in the last commit to remove the default requirement as well as fixing the duplicate chapter menu display. I imagine it would be easy enough to have the chapters show the cue ID plus the cue text if necessary.

Here is an example screenshot
chapters1

Contributor

Chris-DL commented May 22, 2014

@heff, I made a couple of changes in the last commit to remove the default requirement as well as fixing the duplicate chapter menu display. I imagine it would be easy enough to have the chapters show the cue ID plus the cue text if necessary.

Here is an example screenshot
chapters1

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 22, 2014

Member

@Chris-DL, great! I think it would be good to add cue text to this, and increase the width of the chapters menu to account for that. We may need to consider what to do with long titles, though the wrapping that it does by default might be good enough.

@mmcc, do you think you could add 'list' and 'list2' from icomoon to our set?

Member

heff commented May 22, 2014

@Chris-DL, great! I think it would be good to add cue text to this, and increase the width of the chapters menu to account for that. We may need to consider what to do with long titles, though the wrapping that it does by default might be good enough.

@mmcc, do you think you could add 'list' and 'list2' from icomoon to our set?

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc May 22, 2014

Member

Yep, I'll get those added. Reeeally wish we had a timeline for the new Ionicons release, but I'll update the font in the meantime.

Member

mmcc commented May 22, 2014

Yep, I'll get those added. Reeeally wish we had a timeline for the new Ionicons release, but I'll update the font in the meantime.

@silviapfeiffer

This comment has been minimized.

Show comment
Hide comment
@silviapfeiffer

silviapfeiffer May 24, 2014

@heff My example is just that. I don't think the ID needs to be exposed. In fact, we haven't got anything specified in the WebVTT spec yet about how to render Chapters, so any experience that you have and want to share/standardise would be useful!

silviapfeiffer commented May 24, 2014

@heff My example is just that. I don't think the ID needs to be exposed. In fact, we haven't got anything specified in the WebVTT spec yet about how to render Chapters, so any experience that you have and want to share/standardise would be useful!

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 28, 2014

Contributor

@heff, just checking in to see how I should proceed. I did use the cue ID to display the chapter info, but I suppose it really isn't necessary as the cue text could be modified to show the same.

For example:
WEBVTT

1
00:00.700 --> 00:04.110
Chapter 1: [ Heroic music playing for a seagull ]

2
00:04.500 --> 00:05.000
Chapter 2: [ Splash!!! ]

As for accounting for the case of long titles, my css skills aren't the greatest, but if you point me in the right direction I'll put in a fix to get it all wrapped up.

Contributor

Chris-DL commented May 28, 2014

@heff, just checking in to see how I should proceed. I did use the cue ID to display the chapter info, but I suppose it really isn't necessary as the cue text could be modified to show the same.

For example:
WEBVTT

1
00:00.700 --> 00:04.110
Chapter 1: [ Heroic music playing for a seagull ]

2
00:04.500 --> 00:05.000
Chapter 2: [ Splash!!! ]

As for accounting for the case of long titles, my css skills aren't the greatest, but if you point me in the right direction I'll put in a fix to get it all wrapped up.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 28, 2014

Member

@Chris-DL Yeah, actually I think you're right on the cue text part. And ID is actually optional as far as I know, so probably better to not require that by building it into the menu.

For the long titles, I think I'd just make the chapters menu wider. Probably something like this:

.vjs-default-skin .vjs-chapters-button.vjs-menu-button .vjs-menu .vjs-menu-content {
  width: 20em;
}
Member

heff commented May 28, 2014

@Chris-DL Yeah, actually I think you're right on the cue text part. And ID is actually optional as far as I know, so probably better to not require that by building it into the menu.

For the long titles, I think I'd just make the chapters menu wider. Probably something like this:

.vjs-default-skin .vjs-chapters-button.vjs-menu-button .vjs-menu .vjs-menu-content {
  width: 20em;
}
@silviapfeiffer

This comment has been minimized.

Show comment
Hide comment
@silviapfeiffer

silviapfeiffer May 29, 2014

Just confirming: yes, cue IDs are optional. If you want to use markup in the chapters, maybe best to stick with the ones WebVTT knows.

For example:
WEBVTT

1
00:00.700 --> 00:04.110
Chapter 1: Heroic music playing for a seagull

2
00:04.500 --> 00:05.000
Chapter 2: Splash!!!

silviapfeiffer commented May 29, 2014

Just confirming: yes, cue IDs are optional. If you want to use markup in the chapters, maybe best to stick with the ones WebVTT knows.

For example:
WEBVTT

1
00:00.700 --> 00:04.110
Chapter 1: Heroic music playing for a seagull

2
00:04.500 --> 00:05.000
Chapter 2: Splash!!!

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL May 29, 2014

Contributor

@heff I removed the requirement for the cue ID and added in the small style change to widen the menu content. I've included another screenshot so you can view it. Feel free to make any other comments, suggestions, and/or changes.

chapters2

Contributor

Chris-DL commented May 29, 2014

@heff I removed the requirement for the cue ID and added in the small style change to widen the menu content. I've included another screenshot so you can view it. Feel free to make any other comments, suggestions, and/or changes.

chapters2

@heff heff closed this in 95173eb Jun 13, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 13, 2014

Member

Sorry for the delay on this, I was traveling. I just merged it in and made few tweaks to the styles, to make the menu a little wider and center it on the button.

.vjs-default-skin .vjs-chapters-button.vjs-menu-button .vjs-menu .vjs-menu-content {
  width: 24em;
  left: -12em;
}

Thanks for helping fix this, Chris! And thanks for the input, Silvia!

Member

heff commented Jun 13, 2014

Sorry for the delay on this, I was traveling. I just merged it in and made few tweaks to the styles, to make the menu a little wider and center it on the button.

.vjs-default-skin .vjs-chapters-button.vjs-menu-button .vjs-menu .vjs-menu-content {
  width: 24em;
  left: -12em;
}

Thanks for helping fix this, Chris! And thanks for the input, Silvia!

@Chris-DL Chris-DL deleted the Chris-DL:feature/fix-chapters-display branch Jun 13, 2014

@Chris-DL

This comment has been minimized.

Show comment
Hide comment
@Chris-DL

Chris-DL Jun 13, 2014

Contributor

@heff I'm glad I could help out.

Contributor

Chris-DL commented Jun 13, 2014

@heff I'm glad I could help out.

@silviapfeiffer

This comment has been minimized.

Show comment
Hide comment
@silviapfeiffer

silviapfeiffer commented Jun 14, 2014

@heff any time :-)

@Globulopolis

This comment has been minimized.

Show comment
Hide comment
@Globulopolis

Globulopolis Jun 22, 2014

Can we add a markers on the timeline, so we could see which chapter is now playing?!

Globulopolis commented Jun 22, 2014

Can we add a markers on the timeline, so we could see which chapter is now playing?!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 23, 2014

Member

@Globulopolis, I think that could be nice to see but I don't believe there's a standard UI around how the current chapter should be shown on the timeline. Are you able to find other examples of where that's used? I think that should start as a video.js plugin so we can experiment with how that would look/work before making that the default. Would you be interested in making one?

Member

heff commented Jun 23, 2014

@Globulopolis, I think that could be nice to see but I don't believe there's a standard UI around how the current chapter should be shown on the timeline. Are you able to find other examples of where that's used? I think that should start as a video.js plugin so we can experiment with how that would look/work before making that the default. Would you be interested in making one?

@Globulopolis

This comment has been minimized.

Show comment
Hide comment
@Globulopolis

Globulopolis Jun 26, 2014

@heff, look at the picture. When we mouseover on the timeline we can display chapter name in the tooltip. I think we could enlarge the marker when it's mouseovered.
Default state markers
Mouseover(screen from default state)
markers_1

PS! Unfortunately my knowledge in javascript is very bad :-(

Globulopolis commented Jun 26, 2014

@heff, look at the picture. When we mouseover on the timeline we can display chapter name in the tooltip. I think we could enlarge the marker when it's mouseovered.
Default state markers
Mouseover(screen from default state)
markers_1

PS! Unfortunately my knowledge in javascript is very bad :-(

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 27, 2014

Member

@Globulopolis Yeah, that looks good to me. I'd still like to see someone build it as a plugin first.

Member

heff commented Jun 27, 2014

@Globulopolis Yeah, that looks good to me. I'd still like to see someone build it as a plugin first.

@silviapfeiffer

This comment has been minimized.

Show comment
Hide comment
@silviapfeiffer

silviapfeiffer Jun 28, 2014

The proposed approach by @Globulopolis has been common for chapter markers for a while, see http://wiki.whatwg.org/wiki/Use_cases_for_API-level_access_to_timed_tracks#Chapter_Markers for a collection that we put together when researching for the <track> element.

silviapfeiffer commented Jun 28, 2014

The proposed approach by @Globulopolis has been common for chapter markers for a while, see http://wiki.whatwg.org/wiki/Use_cases_for_API-level_access_to_timed_tracks#Chapter_Markers for a collection that we put together when researching for the <track> element.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 1, 2014

Member

Wow, that's a great collection! One potential issue I'm worried about is conflicts with other plugins that put markers on the bar in the same way, e.g. advertising or other timed annotations. I'm not sure exactly how we'd distinguish between them visually, so I don't want to step on any toes by showing the captions markers by default. We could definitely start with an option to turn them on.

Member

heff commented Jul 1, 2014

Wow, that's a great collection! One potential issue I'm worried about is conflicts with other plugins that put markers on the bar in the same way, e.g. advertising or other timed annotations. I'm not sure exactly how we'd distinguish between them visually, so I don't want to step on any toes by showing the captions markers by default. We could definitely start with an option to turn them on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment