Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#24134 closed defect (bug) (fixed)

Video Shortcode ignores sizes and aspect ratio

Reported by: ipstenu's profile Ipstenu Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: has-patch
Focuses: Cc:

Description

I think this may be related to #23955

Uploaded video and resized it:

[video width="300" height="400" mp4="http://elfshot.org/wp-content/uploads/sites/3/2013/04/IMG_0328.mp4"][/video]

Looks fine in the post editor, it's got the right aspects yay.

Then on the live post, the placeholder HTML5 shows sideways (landscape) but when I click play, it shows as full sized to the content.

http://test.ipstenu.org/video-test.2013/ has the example on TwentyThirteen but it happens on all videos on all themes that I've tested. When I viewed source the video tag has the size of the content area, as do media elements.

At revision 24036.

Attachments (4)

24134.diff (566 bytes) - added by kovshenin 12 years ago.
24134.2.diff (3.7 KB) - added by markjaquith 12 years ago.
24134.3.diff (7.8 KB) - added by markjaquith 12 years ago.
24134.4.diff (7.8 KB) - added by markjaquith 12 years ago.
fix a preload oversight for audio.

Download all attachments as: .zip

Change History (34)

#1 @kovshenin
12 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.6

Reproduced this with Twenty Thirteen and Twenty Twelve, but not Twenty Eleven and Twenty Ten.

@kovshenin
12 years ago

#2 @kovshenin
12 years ago

  • Cc wonderboymusic added
  • Keywords has-patch added; needs-patch removed

It looks like MediaElement.js is reading the max-width CSS property and ignoring the specified width and height if it's 100%, which is the case in both Twenty Thirteen and Twenty Twelve. It's what makes both themes responsive.

I think MediaElement.js is making the wrong assumption here, but it's what makes the video responsive.

A workaround would be to wrap the video element with a div, so that even if the video's width is set to 100%, it is still constrained by the parent block, and still responsive because both elements would have a 100% max-width. This, however, does not work for the video height. See 24134.diff.

#3 @Ipstenu
12 years ago

Shouldn't it grab the max width from the video Shortcodes, when applied?

Also max width going larger than the actual video dimensions is sort of poor UI. It makes the videos harder to see, which is terribly evident in vertical vids. That video was saved as 600x800

#4 @Ipstenu
12 years ago

Tested - This patch fixes half the problem (YAY!) and resizes as defined.

It does not fix the problem where the video placeholder loads landscape no matter what. The placeholder issue is way less of a concern than the ignoring of video sizes, though. This needs to be in core to at least get it working as designed.

#5 @nacin
12 years ago

wonderboymusic?

#6 @wonderboymusic
12 years ago

placeholder in landscape.... this problem is kind of a nightmare right now - the getID3 library returns dimensions as resolution_x and resolution_y but doesn't say anything about orientation, it appears that "width" is always the longer side. I need to do some more research on this, the code that deserializes the byte stream into ID3 is unbelievably complex.

#7 @wonderboymusic
12 years ago

From what I am reading, video is always stored wider (portrait is stored as landscape, and might be played back portrait if the app knows what it is doing) - gotta find out if there is transform info stored somewhere in the ID3 stream

#8 @wonderboymusic
12 years ago

Commands like ffmpeg -i IMG_0463.mov will return metadata, including a rotate: 90 declaration, which is what we need.

#10 follow-up: @wonderboymusic
12 years ago

I don't see any solution right now - my only hunch is that reading the Apple-specific metadata for an Apple-generated file would allow us to purposely do a 90 degree CSS3 transform on a portrait video to make it appear correct. That's the best case scenario which I have no code to support. Seems like this data would be in the ID3 library if it was possible to extract reliably.

The main issue is probably the endless number of vendors' metadata to read to know what the rotate of the phone was when the video was recorded. Is there any other scenario where a video would be in portrait orientation, other than when recorded wrong with a phone?

Maybe I've been staring at the code too long and am going insane, am I missing something?

#11 in reply to: ↑ 10 @nacin
12 years ago

Replying to wonderboymusic:

The main issue is probably the endless number of vendors' metadata to read to know what the rotate of the phone was when the video was recorded. Is there any other scenario where a video would be in portrait orientation, other than when recorded wrong with a phone?

Given ID3 is an absurd amount of code, I'd hope it would handle this for us. Since it doesn't, seems like we shouldn't even attempt (considering they've thought of everything else).

#12 @DrewAPicture
12 years ago

Maybe I'm missing something here, but I had the impression the issue is partly a) trying to serve responsive videos and partly b) the video loading with the correct dimensions only to have Mediaelement override with inline styles. Followed by, of course, clicking play and the video loading at max-width which in some cases ends with a vertical video getting scaled up to fit that width.

So I'm not sure what all of this about positioning and 90 degree transforms has to do with it.

Mediaelement is:

1) overriding the dimensions to fit max-width of the container
2) Loading the start screen at a dimensional ratio consistent with max-width, e.g. 604px wide in Twenty Thirteen with a landscape-equivalent height:width ratio regardless of what the actual dimensions are.

#13 follow-up: @wonderboymusic
12 years ago

I never got a portrait mode video to play in portrait in Chrome - tried 8 different videos with setting changes in iMovie.

#14 in reply to: ↑ 13 @DrewAPicture
12 years ago

Replying to wonderboymusic:

I never got a portrait mode video to play in portrait in Chrome - tried 8 different videos with setting changes in iMovie.

This is in FF22: http://screencast.com/t/OlzLhUQypSfs

See the jump on refresh?

#15 follow-up: @wonderboymusic
12 years ago

yeah - the kovshenin patch seems to help that, but I think the bigger issue is that portrait videos don't reliably play as portrait.

Main reason: we actually have no idea when the video is portrait, unless the user changes the dimensions by hand. Even then, the video probably will be laying on its side.

#16 in reply to: ↑ 15 @DrewAPicture
12 years ago

Replying to wonderboymusic:

yeah - the kovshenin patch seems to help that, but I think the bigger issue is that portrait videos don't reliably play as portrait.

Main reason: we actually have no idea when the video is portrait, unless the user changes the dimensions by hand. Even then, the video probably will be laying on its side.

Yeah, and then we have other problem it scaling up to fit max-width of the container: http://screencast.com/t/nQTExszxvNG2

#17 @dfavor
12 years ago

Another consideration is how videos scales responsively (phones, tablets, etc) which is an entire new can of worms.

Possibly a fix for the entire video issue might be using http://fitvidsjs.com to handle responsive scaling.

I'm probably out of line about this seems like the issue of responsive video handling might best be figured into the mix.

#18 @DrewAPicture
12 years ago

@wonderboymusic:

I just realized that we have no difficulty loading videos of all shapes and sizes in the attachment editing UI. Videos are loaded, displayed, and played with the proper dimensions no problem. This is the editing screen for the video I commented on above: http://cl.ly/image/2L173S3A141O

So if we're not trying to enforce responsiveness in the attachment UI, I'm not sure why we need to do it on the front-end. Let core handle outputting the content as intended and themes can handle the rest with fitVids or whatever.

#19 @nacin
12 years ago

What should we do here for 3.6? Anything? Is this a me.js issue? Is this no-one's issue? What are the actual issues? I seem to notice two — portait videos and 100% max-widths.

#20 @Ipstenu
12 years ago

Ignoring Aspect Ratio is clearly a PITA, and a messier problem.

But the shortcode ignoring sizes is, to me at least, the biggest issue, since it's outright breaking how shortcodes are expected to work. If we can fix that, then the aspect ratio can be bumped I think since it'll require some reworking.

Or at least separate the problems into "What can we fix for 3.6 and what can we not fix in time..."

#21 @nacin
12 years ago

So the issue is just how me.js responds when a theme tries to make it responsive width?

#23 @markjaquith
12 years ago

Kovshenin's div wrapper works (tested in 2013) to fix that issue, at least. Still applies.

@markjaquith
12 years ago

@markjaquith
12 years ago

#24 @markjaquith
12 years ago

24134.3.diff Fixes this issue AND #24798 by defaulting preload to metadata for videos.

Additionally:

  • $attr passed through the filter is an actual key/value array instead of an array of attr="val" strings.
  • Respects preload, autoplay, loop, through the shortcode (both audio and video).
  • Includes Kovshenin's div wrapper to fix the responsiveness bug.

@markjaquith
12 years ago

fix a preload oversight for audio.

#25 @markjaquith
12 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 24789:

Fix some sizing issues with video embeds, and improve video/audio embed shortcode flexibility.

  • loop, autoplay, and preload are now available via the shortcode. Use them non-annoyingly, please!
  • Attributes that pass through the filters are now proper key/value pairs, not an array of key="value" strings.
  • preload defaults to metadata for videos. This fixes the vertical video preview and Safari ogv/webm playback issues.
  • Wrap a div around video embeds to combat a ME.js issue with responsive width=100% themes. Props kovshenin.

Fixes #24134, #24798.

#26 @AlphaK
12 years ago

Hi
First post on WP Trac, sorry if I'm doing things wrong...
In changeset [24789], I found it was very annoying having that div wrapped without a class or anything to refer using CSS...
It makes for instance very difficult to center the video, as the wrapped div has to be targeted.

I thing adding a class would greatly help.

#27 follow-up: @Horttcore
12 years ago

I've just discovered that my width and height attributes from my shortcode are overwritten in the wp_video_shortcode function.

I've commented these lines out and its working again.

elseif ( ! is_admin() && $w > $defaults_atts['width'] )
	$w = $defaults_atts['width'];

This [video src="http://xxx/wp-content/uploads/2013/08/ifa-cs.edit_.fx_.012_finish.flv" width="815" height="459"] will result in a video width 640 width and 360 height. Anyone got the same problem?

#28 @SergeyBiryukov
12 years ago

Replying to AlphaK:

In changeset [24789], I found it was very annoying having that div wrapped without a class or anything to refer using CSS...

Replying to Horttcore:

I've just discovered that my width and height attributes from my shortcode are overwritten in the wp_video_shortcode function.

This ticket was closed on a completed milestone. Please create new tickets for new issues.

#29 in reply to: ↑ 27 ; follow-up: @SergeyBiryukov
12 years ago

Replying to Horttcore:

I've just discovered that my width and height attributes from my shortcode are overwritten in the wp_video_shortcode function.

Follow-up: #25312

#30 in reply to: ↑ 29 @AlphaK
11 years ago

Replying to SergeyBiryukov:

Replying to Horttcore:

I've just discovered that my width and height attributes from my shortcode are overwritten in the wp_video_shortcode function.

Follow-up: #25312

Done: Ticket 25896

Last edited 11 years ago by AlphaK (previous) (diff)
Note: See TracTickets for help on using tickets.