Closed Bug 976580 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
QA Contact: rick.eyre
Assignee: nobody → rick.eyre
QA Contact: rick.eyre
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?
Flags: needinfo?(cpearce)
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).
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(jaws)
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.
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.
(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.
'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?
Flags: needinfo?(cpearce)
(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?
Thanks for the tip Jared. 'resizevideocontrols' seems to be working.
Flags: needinfo?(cpearce)
Attachment #8404766 - Flags: review?(giles)
Attachment #8404766 - Flags: review?(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.
Attachment #8404766 - Flags: review?(bzbarsky) → review-
Thanks for review Boris. I've added a TextTrackManager::LoadResource function to add the event listener.
Attachment #8404766 - Attachment is obsolete: true
Attachment #8404766 - Flags: review?(giles)
Attachment #8404880 - Flags: review?(giles)
Attachment #8404880 - Flags: review?(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.
Attachment #8404880 - Flags: review?(bzbarsky) → review+
(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 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.
Attachment #8404880 - Flags: review?(giles) → review+
(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.
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.
Renamed LoadResource() to AddListeners().
Attachment #8404880 - Attachment is obsolete: true
> 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.
(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.
Indeed, thanks for clarifying.
Rebasing against m-c.

Carrying forward r=rillian,bz.
Attachment #8404943 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c7f3704c420c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.