Closed Bug 584259 Opened 11 years ago Closed 10 years ago

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


(Core :: Audio/Video, defect)

Windows 7
Not set





(Reporter: kinetik, Assigned: kinetik)




(2 files, 11 obsolete files)

There's standardization work on the Matroska development list that will enable specify a bunch of different 3D video formats:

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.
Depends on: 584255
I hear that progress has been made on the spec side.
Attached 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
Attached patch patch v0 (obsolete) — Splinter Review
Something like this.
Attached patch nestegg patch v1 (obsolete) — Splinter Review
nestegg patch imported into tree.
Attachment #487724 - Attachment is obsolete: true
Attached 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
Attached patch patch v1.1 (obsolete) — Splinter Review
Forgot to qref.
Attachment #487764 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
Windows build fixes courtesy cpearce.
Attachment #487765 - Attachment is obsolete: true
Attached 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 but fails because m3DVStreaming is NULL due to 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
Attached patch patch v1.4 (obsolete) — Splinter Review
Another minor build fix.
Attachment #487828 - Attachment is obsolete: true
Hi Matthew, I've verified that your patches work perfectly with a 3D WebM video and appropriate drivers and hardware.
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):

Also, here's a link to the current WebM spec discussion:
"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.
Attached patch nestegg patch v2Splinter Review
Adds STEREO_MODE_RIGHT_LEFT, which was added to the Matroska spec recently.
Attachment #487763 - Attachment is obsolete: true
Attached patch patch v1.5 (obsolete) — Splinter Review

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:

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
Attached 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
Attachment #496033 - Flags: review?(chris.double)
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.


+  enum StereoMode {
+  };

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

Otherwise looks good.
Attached 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+
Attached patch patch v1.8Splinter Review
Final changes.  Carrying forward r+ and a+.
Attachment #496395 - Attachment is obsolete: true
The latest build works perfect w/ associated drivers and hardware. Thx.
Attachment #496033 - Flags: review?(chris.double) → review+
Attachment #496033 - Flags: approval2.0?
Closed: 10 years ago
Resolution: --- → FIXED
Can someone with the correct hardware/drivers please mark this verified?
We've got a test rig in the NZ office now, I verified that this works in a nightly yesterday.
Same here.
You need to log in before you can comment on or make changes to this bug.