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

VERIFIED FIXED

Status

()

Core
Audio/Video
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

7 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

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

Comment 2

7 years ago
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(Assignee)

Comment 3

7 years ago
Created attachment 487738 [details] [diff] [review]
patch v0

Something like this.
(Assignee)

Comment 4

7 years ago
Created attachment 487763 [details] [diff] [review]
nestegg patch v1

nestegg patch imported into tree.
Attachment #487724 - Attachment is obsolete: true
(Assignee)

Comment 5

7 years ago
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.
Attachment #487738 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Forgot to qref.
Attachment #487764 - Attachment is obsolete: true
(Assignee)

Comment 7

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

Windows build fixes courtesy cpearce.
Attachment #487765 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
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.
Attachment #487817 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Another minor build fix.
Attachment #487828 - Attachment is obsolete: true

Comment 10

7 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

7 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

7 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

7 years ago
Created attachment 496033 [details] [diff] [review]
nestegg patch v2

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

Comment 14

7 years ago
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.
Attachment #488150 - Attachment is obsolete: true
(Assignee)

Comment 15

7 years ago
Win32 try builds will turn up at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds
/mgregan@mozilla.com-e532ff85bd45
(Assignee)

Comment 16

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

Don't override the StereoMode value if the pref is not set.
Attachment #496037 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 17

7 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

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

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

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

Final changes.  Carrying forward r+ and a+.
Attachment #496395 - Attachment is obsolete: true

Comment 22

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

Updated

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

Updated

7 years ago
Attachment #496033 - Flags: approval2.0?
Attachment #496033 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 23

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

Comment 25

7 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

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