976580 - [WebVTT] Caption sizes aren't changing when the video enters full screen

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

For some reason the caption sizes aren't changing when the video enters full screen. This may be caused because vtt.js now manually calculates the size of the captions instead of through CSS.

Comment 1

[Rick Eyre (:reyre)]

I'm trying to figure out what would be the best way to listen to size changes on the MediaElement would be. Do you have an idea on how to do that Chris?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think the best to craft the CSS for styles of captions such that they Just Work regardless of the size. Can you do something like make the size of the captions being max(10pt, 15% of video height), or just 15% of video height?

I'm not actually sure how to do that with CSS, I don't know CSS very well. Reading up on "responsive web design" might give you some ideas on how to achieve this.

An alternative is to listen on the "resize" event dispatched to the window, but I think you're better of using "responsive design" techniques, so that there's less chance of a state-mismatch bug (i.e. a bug where the state is out of sync with reality causing the script to think it should display captions big but they should be displayed small, etc).

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Jared: Do you know any CSS magic to make video subtitle text responsive, so that we can make it display at a reasonable size when the video is not fullscreen, and then fullscreened? i.e. something like, display at size x% of the video or somesuch?

We may need to use the :-moz-full-screen pseudo class somehow to detect when we've entered fullscreen, and have a different rule for when we're in fullscreen mode.

Comment 4

[Ralph Giles (:rillian)]

Reyre, please correct me if I'm wrong, but I think we can't solve this with CSS for spec reasons. 

http://dev.w3.org/html5/webvtt/#cues-with-video defines a layout algorithm which produces _absolutely_positioned_ CSS boxes, and cannot itself be implemented in CSS.

Therefore to be responsive I think we have to hook the layout code to re-run the webvtt text processing model whenever the video element changes size. My understanding here is limited though; I don't understand why caption size and position are updated on page zoom but not on full screen for example.

Comment 5

[Rick Eyre (:reyre)]

Yeah, that's correct Ralph. So even if we put in responsive CSS for the font-size, etc, we'd still need to recalculate the position of the cue divs since the widths/heights of them would now be different.

The other problem is that the font-size is specified to be 1.5% vh, where vh refers to the height of the video, not the window. I didn't find any way to get that behaviour other then calculating it manually. The spec would have us "redefine the initial containing block" to be the video viewport, which would have vh/vw work correctly according to spec, but that's a lot of work for minimal return I believe.

Those are the main two hurtles to getting a responsive CSS design.

I think the best approach would be to listen for the 'resize' event, like you suggested Chris, and mark the cue's that have been calculated up until that point as dirty. Hopefully we can do that in a lazy way.

Comment 6

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #4)

> I don't understand why caption size and position are updated on page zoom but not on full screen for   > example.

Yeah, that's kind of confusing to me as well.

Comment 7

[Rick Eyre (:reyre)]

'onresize' will trigger when the video enters full screen, but we also have to worry about the other situations where the video frame's size gets changed through JS or other. 'onresize' doesn't seem to work for this. I'm not really sure how we can observe those changes. Do you know Chris?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Rick Eyre (:reyre) from comment #7)
> 'onresize' will trigger when the video enters full screen, but we also have
> to worry about the other situations where the video frame's size gets
> changed through JS or other. 'onresize' doesn't seem to work for this. I'm
> not really sure how we can observe those changes. Do you know Chris?

The video controls have a special 'resizevideocontrols' event that they use, perhaps this can be useful to you?

Comment 9

[Rick Eyre (:reyre)]

Thanks for the tip Jared. 'resizevideocontrols' seems to be working.

Comment 10

[Rick Eyre (:reyre)]

Attachment: v1: Set cue state to dirty when video resize happens r=rillian,bz

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404766 [details] [diff] [review]
v1: Set cue state to dirty when video resize happens r=rillian,bz

>+  mMediaElement->AddEventListener(NS_LITERAL_STRING("resizevideocontrols"),

This is passing a 0-refcount object.  That's really not a safe thing to do.  In particular, something as simple as a stack nsCOMPtr in the callee can lead to a deleted |this| when that callee returns.

You need to add the listener after the constructor returns and you have taken a ref to the TextTrackManager.

>+  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(TextTrackManager,

It's not ambiguous.  Just use NS_DECL_CYCLE_COLLECTION_CLASS.

Comment 12

[Rick Eyre (:reyre)]

Attachment: v2: Set cue state to dirty when video resize happens r=rillian,bz

Thanks for review Boris. I've added a TextTrackManager::LoadResource function to add the event listener.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404880 [details] [diff] [review]
v2: Set cue state to dirty when video resize happens r=rillian,bz

Why LoadResource?  Nothing is being loaded there.

r=me assuming you and Ralph come up with a better name.

Comment 14

[Rick Eyre (:reyre)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Why LoadResource?  Nothing is being loaded there.
> 
> r=me assuming you and Ralph come up with a better name.

Chose it because it's a name I've seen around in Gecko, but yeah it's not the greatest. Maybe InitEventListeners is better. Any ideas Ralph?

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8404880 [details] [diff] [review]
v2: Set cue state to dirty when video resize happens r=rillian,bz

Review of attachment 8404880 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a reasonable idea. r=me for that. I'm still concerned about the performance impact, but let's fix it first.

::: content/html/content/src/TextTrackManager.cpp
@@ -83,5 @@
>  TextTrackManager::TextTrackManager(HTMLMediaElement *aMediaElement)
>    : mMediaElement(aMediaElement)
>    , performedTrackSelection(false)
>  {
> -  MOZ_COUNT_CTOR(TextTrackManager);

How come you removed this? Is it incompatible with cycle collection?

@@ +231,5 @@
>    }
>  }
>  
>  void
> +TextTrackManager::LoadResource()

How about TextTrackManager::AddListeners() ?

::: content/media/TextTrack.cpp
@@ +126,5 @@
>    SetDirty();
>  }
>  
>  void
> +TextTrack::SetCuesDirty()

Would it work to make this SetActiveCuesDirty()? That would limit the overhead.

Comment 16

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #15)

> ::: content/html/content/src/TextTrackManager.cpp
> @@ -83,5 @@
> >  TextTrackManager::TextTrackManager(HTMLMediaElement *aMediaElement)
> >    : mMediaElement(aMediaElement)
> >    , performedTrackSelection(false)
> >  {
> > -  MOZ_COUNT_CTOR(TextTrackManager);
> 
> How come you removed this? Is it incompatible with cycle collection?

I thought that it doesn't need them anymore since it now participates in cycle collection, which would do it for it i.e. MOZ_COUNT_CTOR/DTOR are only for natively refcounted classes. I'm not 100% on that though, I might be wrong.

> @@ +231,5 @@
> >    }
> >  }
> >  
> >  void
> > +TextTrackManager::LoadResource()
> 
> How about TextTrackManager::AddListeners() ?

Sounds good.

> ::: content/media/TextTrack.cpp
> @@ +126,5 @@
> >    SetDirty();
> >  }
> >  
> >  void
> > +TextTrack::SetCuesDirty()
> 
> Would it work to make this SetActiveCuesDirty()? That would limit the
> overhead.

We wouldn't be able to do that because at some point the non-active cues will become active and their display states may already have been computed, which means we have to recompute them again. For example if you watch a video then go into full screen mode and start the video over again you would encounter this problem.

Comment 17

[Rick Eyre (:reyre)]

Sorry, TextTrackManager class now implement nsISupports which uses NS_IMPL_ADDREF and NS_IMPL_RELEASE which means we don't have to use MOZ_COUNT_CTOR/DTOR anymore.

Comment 18

[Rick Eyre (:reyre)]

Attachment: v3: Set cue state to dirty when video resize happens r=rillian,bz

Renamed LoadResource() to AddListeners().

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I thought that it doesn't need them anymore

Correct.  NS_IMPL_ADDREF/RELEASE include addref/release logging, which also tells you when objects are destroyed.

Comment 20

[Rick Eyre (:reyre)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> > I thought that it doesn't need them anymore
> 
> Correct.  NS_IMPL_ADDREF/RELEASE include addref/release logging, which also
> tells you when objects are destroyed.

Great. Thanks for the confirmation Boris.

Comment 21

[Ralph Giles (:rillian)]

Indeed, thanks for clarifying.

Comment 22

[Rick Eyre (:reyre)]

Attachment: v4: Set cue state to dirty when video resize happens r=rillian,bz

Rebasing against m-c.

Carrying forward r=rillian,bz.

Comment 23

[Rick Eyre (:reyre)]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=e22451401398

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f3704c420c

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7f3704c420c

Comment 26

[Ada [:adalucinet]]

Verified as fixed with latest Aurora (Build ID: 20140604004003) on Windows 7 x64, Mac OS X 10.9.3 and Ubuntu 13.04 x64:

Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0