584259 - Add experimental support for packed format 3D video to libnestegg/WebMReader

Status: VERIFIED FIXED

Description

[Matthew Gregan [:kinetik]]

There's standardization work on the Matroska development list that will enable specify a bunch of different 3D video formats: http://lists.matroska.org/pipermail/matroska-devel/2010-July/003741.html

And we have a patch from NVIDIA to add support for side-by-side packed 3D to the rendering code over in bug 584255.

Once the Matroska side is nailed down, we should add experimental support to libnestegg/WebMReader to expose this metadata to the render code so it can enable 3D rendering when possible.

Comment 1

[Christopher Blizzard (:blizzard)]

I hear that progress has been made on the spec side.

Comment 2

[Matthew Gregan [:kinetik]]

Created attachment 487724 [details] [diff] [review]
nestegg patch v0

I think this is all that's required on the nestegg side.  I won't push this to upstream nestegg until we've nailed down exactly what we need.  I'm waiting to hear back from Alok about mapping these values to the ones wanted by the NVIDIA 3D API.

For the decoder/playback engine, it looks like storing the stereo mode in nsVideoInfo and then creating the appropriate format Image from the ImageContainer is the right way to pass this data into the Layers rendering code.

Comment 3

[Matthew Gregan [:kinetik]]

Created attachment 487738 [details] [diff] [review]
patch v0

Something like this.

Comment 4

[Matthew Gregan [:kinetik]]

Created attachment 487763 [details] [diff] [review]
nestegg patch v1

nestegg patch imported into tree.

Comment 5

[Matthew Gregan [:kinetik]]

Created attachment 487764 [details] [diff] [review]
patch v1

Updated with other modes.

This is all completely untested, but it should work.  Apply both patches, set gfx.3d_video.enabled to true, and play a video with the appropriate StereoMode value in the container.

Comment 6

[Matthew Gregan [:kinetik]]

Created attachment 487765 [details] [diff] [review]
patch v1.1

Forgot to qref.

Comment 7

[Matthew Gregan [:kinetik]]

Created attachment 487817 [details] [diff] [review]
patch v1.2

Windows build fixes courtesy cpearce.

Comment 8

[Matthew Gregan [:kinetik]]

Created attachment 487828 [details] [diff] [review]
patch v1.3

Another fix.  With these patches, an appropriately generated video, the pref enabled, and Layers mode forced to D3D9, this gets as far as http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/ImageLayerD3D9.cpp#192 but fails because m3DVStreaming is NULL due to http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/Nv3DVUtils.cpp#86 failing.  This probably means I don't have the appropriate drivers or hardware on the machine I tested on.

Comment 9

[Matthew Gregan [:kinetik]]

Created attachment 488150 [details] [diff] [review]
patch v1.4

Another minor build fix.

Comment 10

[waveletcoeff]

Hi Matthew, I've verified that your patches work perfectly with a 3D WebM video and appropriate drivers and hardware.

Comment 11

[Matthew Gregan [:kinetik]]

Thanks for verifying!  We'll need to get our hands on some testing equipment so we can work on this locally in the near future.

A couple of notes:

We can probably remove the gfx.3d_video.enabled pref and allow the code to rely on: 3D driver support being present and the appropriate StereoMode flag in the container.

I also need to verify that the current behaviour wrt this note in the Matroska spec notes:

  The pixel count of the track (PixelWidth/PixelHeight) should be the raw amount
  of pixels (for example 3840x1080 for full HD side by side) and the
  DisplayWidth/Height in pixels should be the amount of pixels for one plane
  (1920x1080 for that full HD stream).

...because I have a feeling that the current code will do the wrong thing in this case.  It looks like DisplayWidth/Height needs to be special cased for files with StereoMode set. :-(

Test Win32 builds of the current patches are here (until they expire in 2 weeks): http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3d09ea745f0a/tryserver-win32/

Also, here's a link to the current WebM spec discussion: http://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/94c71028b688b998/561ed3d008d19568#561ed3d008d19568

Comment 12

[Steve Lhomme]

"It looks like DisplayWidth/Height needs to be special cased for
files with StereoMode set."

No, the example you mentioned is when the video is not expanded. In the more common (current use in IPTV for example) each side is shrinked horizontally to fit a normal frame to decode. So the pixel values are 1920/1080 and the display values are also 1920/1080. Knowing that it's horizontal side by side you know you have to cut in the middle. And then the display values tell you how these rectangles are supposed to be displayed.

Comment 13

[Matthew Gregan [:kinetik]]

Created attachment 496033 [details] [diff] [review]
nestegg patch v2

Adds STEREO_MODE_RIGHT_LEFT, which was added to the Matroska spec recently.

Comment 14

[Matthew Gregan [:kinetik]]

Created attachment 496037 [details] [diff] [review]
patch v1.5

Adds STEREO_MODE_RIGHT_LEFT.

Removes the gfx.3d_video.enabled pref completely and relies on the stereo mode being set on the image.

Adds a pref which is read during metadata load in nsWebMReader that allows a particular stereo mode to be forced for testing files that don't have a StereoMode element.  This is a temporary measure until the tools (and YouTube) have been modified to support adding this element.  The pref is an integer pref called "media.webm.force_stereo_mode", with the following valid values:

1 = LEFT_RIGHT
2 = RIGHT_LEFT
3 = TOP_BOTTOM
4 = BOTTOM_TOP
anything else = MONO

As with the earlier patches, you also need to force D3D9 layers mode (if you default to D3D10) due to only having support for stereo video in the D3D9 code.  Bug 617220 covers adding support to D3D10.

Comment 15

[Matthew Gregan [:kinetik]]

Win32 try builds will turn up at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds
/[e-mail]-e532ff85bd45

Comment 16

[Matthew Gregan [:kinetik]]

Created attachment 496374 [details] [diff] [review]
patch v1.6

Don't override the StereoMode value if the pref is not set.

Comment 17

[Matthew Gregan [:kinetik]]

Comment on attachment 496374 [details] [diff] [review]
patch v1.6

Bas might be better for the D3D9 review, but I picked Robert because he can review both modules.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The StereoMode enum could move from PlanarYCbCrImage::Data to the mozilla::layers namespace. That would make the code more concise, and at some point we'll probably want to reuse it for other image types.

nsVideoInfo:

+  enum StereoMode {
+    STEREO_MODE_MONO,
+    STEREO_MODE_LEFT_RIGHT,
+    STEREO_MODE_RIGHT_LEFT,
+    STEREO_MODE_BOTTOM_TOP,
+    STEREO_MODE_TOP_BOTTOM
+  };

We can just use the mozilla::layers::StereoMode instead.

Otherwise looks good.

Comment 19

[Matthew Gregan [:kinetik]]

Created attachment 496395 [details] [diff] [review]
patch v1.7

With roc's changes.  Also prefixed the Nv3DUtils stereo mode stuff with Nv_.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 496395 [details] [diff] [review]
patch v1.7

Drop a 'using namespace mozilla::layers;" into nsWebMReader.cpp and lose those ugly prefixes

Comment 21

[Matthew Gregan [:kinetik]]

Created attachment 496400 [details] [diff] [review]
patch v1.8

Final changes.  Carrying forward r+ and a+.

Comment 22

[waveletcoeff]

The latest build works perfect w/ associated drivers and hardware. Thx.

Comment 23

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/28f582d9b6d8
http://hg.mozilla.org/mozilla-central/rev/df3122f03328

Comment 24

[Anthony Hughes (:ashughes) [GFX][QA][Mentor]]

Can someone with the correct hardware/drivers please mark this verified?

Comment 25

[Matthew Gregan [:kinetik]]

We've got a test rig in the NZ office now, I verified that this works in a nightly yesterday.

Comment 26

[Steve Lhomme]

Same here.