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

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

Assignee

Description

9 years ago
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.
Assignee

Updated

9 years ago
Depends on: 584255
I hear that progress has been made on the spec side.
Assignee

Comment 2

9 years ago
Posted patch nestegg patch v0 (obsolete) — Splinter Review
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee

Comment 3

9 years ago
Posted patch patch v0 (obsolete) — Splinter Review
Something like this.
Assignee

Comment 4

9 years ago
Posted patch nestegg patch v1 (obsolete) — Splinter Review
nestegg patch imported into tree.
Attachment #487724 - Attachment is obsolete: true
Assignee

Comment 5

9 years ago
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #487738 - Attachment is obsolete: true
Assignee

Comment 6

9 years ago
Posted patch patch v1.1 (obsolete) — Splinter Review
Forgot to qref.
Attachment #487764 - Attachment is obsolete: true
Assignee

Comment 7

9 years ago
Posted patch patch v1.2 (obsolete) — Splinter Review
Windows build fixes courtesy cpearce.
Attachment #487765 - Attachment is obsolete: true
Assignee

Comment 8

9 years ago
Posted patch patch v1.3 (obsolete) — Splinter Review
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.
Attachment #487817 - Attachment is obsolete: true
Assignee

Comment 9

9 years ago
Posted patch patch v1.4 (obsolete) — Splinter Review
Another minor build fix.
Attachment #487828 - Attachment is obsolete: true

Comment 10

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

Comment 11

9 years ago
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/mgregan@mozilla.com-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

9 years ago
"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.
Assignee

Comment 13

9 years ago
Adds STEREO_MODE_RIGHT_LEFT, which was added to the Matroska spec recently.
Attachment #487763 - Attachment is obsolete: true
Assignee

Comment 14

9 years ago
Posted patch patch v1.5 (obsolete) — Splinter Review
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.
Attachment #488150 - Attachment is obsolete: true
Assignee

Comment 16

9 years ago
Posted patch patch v1.6 (obsolete) — Splinter Review
Don't override the StereoMode value if the pref is not set.
Attachment #496037 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #496033 - Flags: review?(chris.double)
Assignee

Comment 17

9 years ago
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.
Attachment #496374 - Flags: review?(roc)
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.
Assignee

Comment 19

9 years ago
Posted patch patch v1.7 (obsolete) — Splinter Review
With roc's changes.  Also prefixed the Nv3DUtils stereo mode stuff with Nv_.
Attachment #496374 - Attachment is obsolete: true
Attachment #496395 - Flags: review?(roc)
Attachment #496374 - Flags: review?(roc)
Comment on attachment 496395 [details] [diff] [review]
patch v1.7

Drop a 'using namespace mozilla::layers;" into nsWebMReader.cpp and lose those ugly prefixes
Attachment #496395 - Flags: review?(roc)
Attachment #496395 - Flags: review+
Attachment #496395 - Flags: approval2.0+
Assignee

Comment 21

9 years ago
Posted patch patch v1.8Splinter Review
Final changes.  Carrying forward r+ and a+.
Attachment #496395 - Attachment is obsolete: true

Comment 22

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

Updated

9 years ago
Attachment #496033 - Flags: review?(chris.double) → review+
Assignee

Updated

9 years ago
Attachment #496033 - Flags: approval2.0?
Assignee

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/28f582d9b6d8
http://hg.mozilla.org/mozilla-central/rev/df3122f03328
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Can someone with the correct hardware/drivers please mark this verified?
Assignee

Comment 25

9 years ago
We've got a test rig in the NZ office now, I verified that this works in a nightly yesterday.
Status: RESOLVED → VERIFIED

Comment 26

9 years ago
Same here.
You need to log in before you can comment on or make changes to this bug.