584255 - Enable HTML5 Stereo Rendering

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Atul]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; .NET4.0C; .NET4.0E)
Build Identifier: Firefox 4.0 beta

Support for html5 stereo rendering

Reproducible: Always

Comment 1

[Atul]

Attachment: Patch to enable stereo rendering

Comment 2

[Bas Schouten (:bas.schouten)]

For a first pass, there's several things that would need to change:

- We use the K&R bracing style in our code { and } go on the same lines so elses are cuddled.
- We need to be careful with CoInitialize, we need a balancing CoUninitialize call and ordering matters there!
- The majority of the NvD3D specific code I'd like to see go into an NvD3DUtils.h/cpp file pair. And then we can call it for example like:

 if (mIs3DEnabled)
 {
   NvD3DUtils::LoadNv3DVStreaming();
 }

As far as I can see a 3DStreaming instance is device specific, so we may need to instantiate something for that. Or you might have a better idea?
- Local stack variables do not require any prefixes

Comment 3

[Atul]

Thanks for the comments, Bas. I'll go ahead and make the changes you suggested. Some comments below:

> For a first pass, there's several things that would need to change:
> - We use the K&R bracing style in our code { and } go on the same lines so
> elses are cuddled.
> - We need to be careful with CoInitialize, we need a balancing CoUninitialize
> call and ordering matters there!

Where would you suggest I add the CoUninitialize()? The layer manager has an Initialize(), but I don't see a corresponding release\uninitialize.

> - The majority of the NvD3D specific code I'd like to see go into an
> NvD3DUtils.h/cpp file pair. And then we can call it for example like:
>  if (mIs3DEnabled)
>  {
>    NvD3DUtils::LoadNv3DVStreaming();
>  }

I will go ahead and move all the code into a separate set of files and a new class, and then have an object of this class instantiated in the layer manager. Would that work for you?

> As far as I can see a 3DStreaming instance is device specific, so we may need
> to instantiate something for that. Or you might have a better idea?

The 3DStreaming instance is created once when the layer manager Initialize() function gets called. It will not re-create the dll instance if it already has been created by a prior Initialize() call. The 3DVSetDevice() function will accept a device pointer, so that ties the dll instance to some d3d device. Were you concerned about some specific case?

> - Local stack variables do not require any prefixes

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to comment #3)
> Thanks for the comments, Bas. I'll go ahead and make the changes you suggested.
> Some comments below:
> Where would you suggest I add the CoUninitialize()? The layer manager has an
> Initialize(), but I don't see a corresponding release\uninitialize.

I think the destructor ought to work here.

> I will go ahead and move all the code into a separate set of files and a new
> class, and then have an object of this class instantiated in the layer manager.
> Would that work for you?

Sounds good

> 
> The 3DStreaming instance is created once when the layer manager Initialize()
> function gets called. It will not re-create the dll instance if it already has
> been created by a prior Initialize() call. The 3DVSetDevice() function will
> accept a device pointer, so that ties the dll instance to some d3d device. Were
> you concerned about some specific case?

We use a seperate LayerManager for each tab (which is its own window). Therefor we have to deal with multiple layer managers and multiple IDirect3DDevice9 objects. As as I could see none of the members are static - so each new LayerManager would, upon initialization re-create the DLL and tie it to a device, correct?

Comment 5

[Atul]

(In reply to comment #4)
> We use a seperate LayerManager for each tab (which is its own window). Therefor
> we have to deal with multiple layer managers and multiple IDirect3DDevice9
> objects. As as I could see none of the members are static - so each new
> LayerManager would, upon initialization re-create the DLL and tie it to a
> device, correct?

Yes, that is correct. Each new layer manager would (upon first time Init) create an instance of the DLL. Since the d3d9 device will be different from one tab to the next, we would need separate DLL instances to tie to the separate devices, right?

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> (In reply to comment #4)
> > We use a seperate LayerManager for each tab (which is its own window). Therefor
> > we have to deal with multiple layer managers and multiple IDirect3DDevice9
> > objects. As as I could see none of the members are static - so each new
> > LayerManager would, upon initialization re-create the DLL and tie it to a
> > device, correct?
> 
> Yes, that is correct. Each new layer manager would (upon first time Init)
> create an instance of the DLL. Since the d3d9 device will be different from one
> tab to the next, we would need separate DLL instances to tie to the separate
> devices, right?

Yep, if your okay with that, that's fine!

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I would just CoInitialize once in gfx startup and Uninitialize in gfx shutdown.  

Also, by "DLL instance" you just mean "in process COM server/object", right?  That is, we're just calling CoCreateInstance and letting it do whatever's needed under the hood, right?

Comment 8

[Atul]

(In reply to comment #7)
> I would just CoInitialize once in gfx startup and Uninitialize in gfx shutdown. 

Where would you recommend would be the best place to do this? I see gfxWindowsPlatform::gfxWindowsPlatform() and gfxWindowsPlatform::~gfxWindowsPlatform() as potential places where I could do the CoInitalize() and CoUninitialize() respectively, but not sure if that was what you had in mind.

> Also, by "DLL instance" you just mean "in process COM server/object", right? 
> That is, we're just calling CoCreateInstance and letting it do whatever's
> needed under the hood, right?

Yes, we are calling CoCreateInstance to create a new instance of the COM object specified by our clsid, and letting it do whatever is needed under the hood. So yes, new instace of the COM object is a more accurate way to put it.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> (In reply to comment #7)
> > I would just CoInitialize once in gfx startup and Uninitialize in gfx shutdown. 
> 
> Where would you recommend would be the best place to do this? I see
> gfxWindowsPlatform::gfxWindowsPlatform() and
> gfxWindowsPlatform::~gfxWindowsPlatform() as potential places where I could do
> the CoInitalize() and CoUninitialize() respectively, but not sure if that was
> what you had in mind.

Yep, those would be fine -- the gfxWindowsPlatform singleton is created from our startup/shutdown routines, so the lifetime would be correct.

Though something to consider, now that I think about it; we should watch what happens to win32 startup time with this, to make sure that CoInitialize doesn't add any significant overhead.  (If it does we can just do it lazily, not a big deal.)

Comment 10

[Atul]

Attachment: Patch to enable stereo rendering v2

Comment 11

[Atul]

I've incorporated the re-architecture suggestions in the new patch that I just
uploaded. Thanks

Comment 12

[Bas Schouten (:bas.schouten)]

We need this for our D2D users to be able to use this.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 462974 [details] [diff] [review]
Patch to enable stereo rendering v2

Getting better! Trying to give some quick review feedback here!

In general, it seems I was unclear about our bracing style, although the } else { is now correct. The opening brace also goes on the same line, i.e.

if (foo) {
  dosomething();
} else {
  dosomethingelse();
}

A lot of the logging could probably also be done using NS_WARNING macro's (only in the case where we really don't expect things to fail! i.e. things that will fail for anyone with no 3D vision libraries installed can just use the logging you're using right now.

>@@ -175,16 +178,63 @@ ImageLayerD3D9::RenderLayer()
>      * We always upload a 4 component float, but the shader will
>      * only use the the first component since it's declared as a 'float'.
>      */
>     opacity[0] = GetOpacity();
>     device()->SetPixelShaderConstantF(0, opacity, 1);
> 
>     mD3DManager->SetShaderMode(LayerManagerD3D9::YCBCRLAYER);
> 
>+    INv3DVStreaming *streaming = mD3DManager->Get3DVStreaming();
>+    PRBool is3DEnabled = mD3DManager->Is3DEnabled();
>+    nsCOMPtr<nsIConsoleService>
> ....
>+
>+    } // is3DEnabled && streaming
>+

I'd like to see most of the code here go into the Nv3DUtils object and only 1 or 2 lines in here. Feel free to do console output in there and such.

>diff --git a/gfx/layers/d3d9/LayerManagerD3D9.cpp b/gfx/layers/d3d9/LayerManagerD3D9.cpp
>--- a/gfx/layers/d3d9/LayerManagerD3D9.cpp
>+++ b/gfx/layers/d3d9/LayerManagerD3D9.cpp
>@@ -63,28 +65,95 @@ typedef IDirect3D9* (WINAPI*Direct3DCrea
> );
> 
> 
> LayerManagerD3D9::LayerManagerD3D9(nsIWidget *aWidget)
> {
>     mWidget = aWidget;
>     mCurrentCallbackInfo.Callback = NULL;
>     mCurrentCallbackInfo.CallbackData = NULL;
>+    mNv3DVUtils = NULL;
>+    mIs3DEnabled = PR_FALSE;

These two are class members of the LayerManagerD3D9 and can go into the initializer list.

> }
> 
> LayerManagerD3D9::~LayerManagerD3D9()
> {
>+  if (mNv3DVUtils)
>+  {
>+    mNv3DVUtils->Release();
>+    delete mNv3DVUtils;
>+    mNv3DVUtils = NULL; 
>+  }

Is it possible to just release from the Nv3DVUtils object destructor? In any case this member should be an nsAutoPtr<Nv3DUtils>. Then we will not need any added destructor code as far as I'm concerned.

>+  if (mNv3DVUtils && mIs3DEnabled)

Checking just Nv3DVUtils here should be fine I think? It will be NULL is mIs3DEnabled, and even if it isn't it's fine if it gets initialized I suppose?

>+  {
>+    rv = mNv3DVUtils->Initialize();
>+    if (!rv)
>+    {
>+      if (consoleCustom) 
>+      {
>+        nsString msg;
>+        msg += NS_LITERAL_STRING("Could not Initialize Nv3DVUtils.\n");

If no other error handling code is required I'm also okay with putting the log message in the Initialize function, and ignoring the return value here, I'm also fine with it returning void as long as it internally logs errors.

>+  /*
>+   * Do some post-device creation setup
>+   */
>+  if (mNv3DVUtils && mIs3DEnabled)

In this case too just checking mNv3DVUtils is probably fine?
>diff --git a/gfx/layers/d3d9/Nv3DVUtils.cpp b/gfx/layers/d3d9/Nv3DVUtils.cpp
>new file mode 100644
>--- /dev/null
>+++ b/gfx/layers/d3d9/Nv3DVUtils.cpp
>@@ -0,0 +1,218 @@
>+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Corporation code.

I believe you can put NVidia in here. But I'm not much of a legal guy.

>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Bas Schouten <[e-mail]>

Feel free to take the credit here! :-) Same goes for the header.

>+  hr = CoCreateInstance(CLSID_NV3DVStreaming, NULL, CLSCTX_INPROC_SERVER, IID_INV3DVStreaming, (void**)(&m3DVStreaming));

m3DVStreaming can be an nsRefPtr<INV3DVStreaming> member, then you can use getter_AddRefs(m3DVStreaming) to pass it in, if this doesn't work for some reason, you can create an INV3DVStreaming * on the stack, assign that to m3DVStreaming and Release it right away. We generally prefer automatic pointers. All the calls to release should go away then, releasing can just be done by assigning NULL to m3DVStreaming.

>+void
>+Nv3DVUtils::Release()
>+{
>+  if (m3DVStreaming)
>+  {
>+    m3DVStreaming->Nv3DVRelease();
>+    m3DVStreaming->Release();
>+  }
>+}

I'd prefer a name a bit more verbose than Release, since it could be easily confused with a refcounting class. ReleaseResources perhaps?

Comment 14

[Atul]

Attachment: Patch to enable html5 stereo rendering

Thanks for comments, Bas. 

For now, the license section in the 2 new files is still the same Mozilla one that is used in the other files (we still need to work out on our side what needs to go there). Other than that, I've updated the patch per your comments: I moved most of the logic into the Nv3DVUtils class, along with other changes that you suggested. 

Thanks.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 463265 [details] [diff] [review]
Patch to enable html5 stereo rendering

>diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in
>--- a/gfx/layers/Makefile.in
>+++ b/gfx/layers/Makefile.in
>@@ -80,24 +80,26 @@ CPPSRCS = \
>         ImageLayerOGL.cpp \
>         LayerManagerOGL.cpp \
>         ThebesLayerOGL.cpp \
>         $(NULL)
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> ifdef MOZ_ENABLE_D3D9_LAYER
> EXPORTS += LayerManagerD3D9.h
>+EXPORTS += Nv3DVUtils.h

I don't think this needs to be exported; it's only used in layers code (in that srcdir).

>+    /*
>+     * Send 3d control data and metadata
>+     */
>+    if (mD3DManager->Is3DEnabled() && mD3DManager->GetNv3DVUtils()) {
>+      mD3DManager->GetNv3DVUtils()->SendNv3DVControl(STEREO_MODE_RIGHT_LEFT, true, FIREFOX_3DV_APP_HANDLE);
>+      
>+      nsRefPtr<IDirect3DSurface9> renderTarget;
>+      device()->GetRenderTarget(0, getter_AddRefs(renderTarget));
>+      mD3DManager->GetNv3DVUtils()->SendNv3DVMetaData((unsigned int)yuvImage->mSize.width, 
>+        (unsigned int)yuvImage->mSize.height, (HANDLE)(yuvImage->mYTexture), (HANDLE)(renderTarget));

I'm not sure I fully understand the API, but I'm assuming that this is saying "when texture mYTexture is drawn to renderTarget, treat it as RIGHT_LEFT stereo"?  Do we need to do anything for the Cb/Cr textures, or is one texture having the stereo bit set sufficient to treat all of them in the same draw command as being stereo?

Looks fine otherwise, though!

Comment 16

[Atul]

Thanks for the comments, Vlad.

> >+EXPORTS += Nv3DVUtils.h
> I don't think this needs to be exported; it's only used in layers code (in that
> srcdir).

I was running into compilation issues with the header file not getting recognized by the layer manager header, hence I assumed it needed to be added to the exports. Is there some other way to make sure that the header file is visible to LayerManagerD3D9?

> >+    /*
> >+     * Send 3d control data and metadata
> >+     */
> >+    if (mD3DManager->Is3DEnabled() && mD3DManager->GetNv3DVUtils()) {
> >+      mD3DManager->GetNv3DVUtils()->SendNv3DVControl(STEREO_MODE_RIGHT_LEFT, true, FIREFOX_3DV_APP_HANDLE);
> >+      
> >+      nsRefPtr<IDirect3DSurface9> renderTarget;
> >+      device()->GetRenderTarget(0, getter_AddRefs(renderTarget));
> >+      mD3DManager->GetNv3DVUtils()->SendNv3DVMetaData((unsigned int)yuvImage->mSize.width, 
> >+        (unsigned int)yuvImage->mSize.height, (HANDLE)(yuvImage->mYTexture), (HANDLE)(renderTarget));
> I'm not sure I fully understand the API, but I'm assuming that this is saying
> "when texture mYTexture is drawn to renderTarget, treat it as RIGHT_LEFT
> stereo"?  Do we need to do anything for the Cb/Cr textures, or is one texture
> having the stereo bit set sufficient to treat all of them in the same draw
> command as being stereo?
> Looks fine otherwise, though!

These calls in ImageLayerD3D9::RenderLayer() actually route everything to the Nv3DVUtils class, and most of the logic is handled there (the calls to the COM object are actually in the code in Nv3DVUtils). Regarding the API itself, the SetControl call specifies some information about the actual format of the stereo data. And as you mentioned, the SetMetadata call indicates that when the specified texture is rendered to the specified render target, we consider that to be a call with stereo data (of the format specified in the earlier SetControl call).

We don't need to consider the Cb and Cr data, as long as we are only dealing with 4:2:0 YUV data (that is, Cb and Cr are half the width and half the height of the luma). Do you envision this ever changing to the point where we might be dealing with 4:2:2 (Cb and Cr are same height as Y, but half the width) or 4:4:4 (Y, Cb, Cr are the same width and height) style data?

Thanks.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #16)
> Thanks for the comments, Vlad.
> 
> > >+EXPORTS += Nv3DVUtils.h
> > I don't think this needs to be exported; it's only used in layers code (in that
> > srcdir).
> 
> I was running into compilation issues with the header file not getting
> recognized by the layer manager header, hence I assumed it needed to be added
> to the exports. Is there some other way to make sure that the header file is
> visible to LayerManagerD3D9?

Oh, ha, this is because of the really funky (and broken, we never should have done this!) multi-dir and VPATH setup in gfx/layers.  Adding -I$(srcdir)/d3d9 to LOCAL_INCLUDES should do it, I think.

> We don't need to consider the Cb and Cr data, as long as we are only dealing
> with 4:2:0 YUV data (that is, Cb and Cr are half the width and half the height
> of the luma). Do you envision this ever changing to the point where we might be
> dealing with 4:2:2 (Cb and Cr are same height as Y, but half the width) or
> 4:4:4 (Y, Cb, Cr are the same width and height) style data?

Oh right, forgot that this is 4:2:0.  I don't think we'll deal with 4:2:2 or 4:4:4 anytime soon as most things want to work with 4:2:0, so I wouldn't worry about that.

Comment 18

[Atul]

> Oh, ha, this is because of the really funky (and broken, we never should have
> done this!) multi-dir and VPATH setup in gfx/layers.  Adding -I$(srcdir)/d3d9
> to LOCAL_INCLUDES should do it, I think.

Makefile.in in gfx\layers does not have a LOCAL_INCLUDES. But, I'm assuming it is ok for me to add a new line for LOCAL_INCLUDES in this makefile, which is something like what I've put below, right?

LOCAL_INCLUDES = \
    -I$(srcdir)/d3d9/. \
    $(NULL)

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, exactly; don't even have to split it up on multiple lines (and probably don't include the /. -- it's probably fine but I'm terrified of weirdness).  Just 'LOCAL_INCLUDES = -I$(srcdir)/d3d9' should be fine.

Comment 20

[Atul]

Attachment: Patch to enable html5 stereo rendering

Updated the patch to add LOCAL_INCLUDES in the Makefile.

Comment 21

[Atul]

Bas, did you have any comments about the latest patch, and were there any changes that you would like me to make? 

(regarding the licence section of the 2 new files, we can probably just keep that the way it is for now, if that is ok?)

Thanks.

Comment 22

[Atul]

Attachment: Patch to enable html5 stereo rendering

Update to the way the new header was being included, to get things to compile properly.

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to comment #21)
> Bas, did you have any comments about the latest patch, and were there any
> changes that you would like me to make? 
> 
> (regarding the licence section of the 2 new files, we can probably just keep
> that the way it is for now, if that is ok?)
> 
> Thanks.

I just asked Matthew, and Theora also supports 4:2:2 and 4:4:4. So possibly we need to add a check for that or something? I sadly haven't had time to examine rest of the patch yet.

Comment 24

[Atul]

Attachment: Patch to enable html5 stereo rendering

Updating the html5 stereo rendering patch based on the re-arch done by Bas (to use a single device for all LayerManagers).

Comment 25

[Atul]

Attachment: Patch to enable html5 stereo rendering

Forgot to add the user preference part to the earlier patch ... Updated.

Comment 26

[Bas Schouten (:bas.schouten)]

Comment on attachment 465524 [details] [diff] [review]
Patch to enable html5 stereo rendering

Looking better every time! Just some small comments.

>@@ -174,19 +176,25 @@ SwapChainD3D9::Reset()
>   mSwapChain = nsnull;
> }
> 
> #define HAS_CAP(a, b) (((a) & (b)) == (b))
> #define LACKS_CAP(a, b) !(((a) & (b)) == (b))
> 
> DeviceManagerD3D9::DeviceManagerD3D9()
>   : mHasDynamicTextures(false)
>+  , mNv3DVUtils(NULL) 
This is not needed. The nsAutoPtr template class constructor will NULL this.

> {
> }
> 
>+DeviceManagerD3D9::~DeviceManagerD3D9()
>+{
>+  mNv3DVUtils = NULL;
>+}
>+
This destructor can go away, the nsAutoPtr template class destructor will be called on DeviceManagerD3D9 destruction which will also delete the pointer.

> class THEBES_API DeviceManagerD3D9
> {
> public:
>   DeviceManagerD3D9();
>+  ~DeviceManagerD3D9();
> 
>+  /** 
>+   * Return pointer to the Nv3DVUtils instance. Re-direct to mDeviceManager.
>+   */ 
>+  Nv3DVUtils *GetNv3DVUtils()  { return (mDeviceManager ? mDeviceManager->GetNv3DVUtils() : NULL); } 
Although it's not too important I don't think the parenthesis here really make it more readable.

>+/**
>+ * Release resources used by the COM Object, and then release 
>+ * the COM Object (nsRefPtr gets released by setting to NULL) 
>+ *
>+ */
>+void
>+Nv3DVUtils::UnInitialize()
>+{
>+  if (m3DVStreaming) {
>+    m3DVStreaming->Nv3DVRelease();
>+    m3DVStreaming = NULL;
>+  }
>+}
You could let the class destructor take care of destroying the m3DVStreaming object through the nsRefPtr destructor

Comment 27

[Bas Schouten (:bas.schouten)]

Oh, we spoke about this patch briefly, the call to Initialize should probably be deferred to when it's first used, otherwise this could have startup performance impact for people not using 3D video.

Comment 28

[Atul]

Thanks for the comments. I'm in the process of making the updates.

> >+/**
> >+ * Release resources used by the COM Object, and then release 
> >+ * the COM Object (nsRefPtr gets released by setting to NULL) 
> >+ *
> >+ */
> >+void
> >+Nv3DVUtils::UnInitialize()
> >+{
> >+  if (m3DVStreaming) {
> >+    m3DVStreaming->Nv3DVRelease();
> >+    m3DVStreaming = NULL;
> >+  }
> >+}
> You could let the class destructor take care of destroying the m3DVStreaming
> object through the nsRefPtr destructor

If I set m3DVStreaming to NULL as above, wouldn't that cause the object to get destroyed through the nsRefPtr destructor? Or are you suggesting that I can just move all this code from Uninitialize() into the 3DVUtils destructor? (I wanted to have an UnInitialize() to make sure that it pairs up with the Initialize(), which is where the 3D Streaming COM object initialization happens)

Comment 29

[Bas Schouten (:bas.schouten)]

(In reply to comment #28)
> Thanks for the comments. I'm in the process of making the updates.
> 
> > >+/**
> > >+ * Release resources used by the COM Object, and then release 
> > >+ * the COM Object (nsRefPtr gets released by setting to NULL) 
> > >+ *
> > >+ */
> > >+void
> > >+Nv3DVUtils::UnInitialize()
> > >+{
> > >+  if (m3DVStreaming) {
> > >+    m3DVStreaming->Nv3DVRelease();
> > >+    m3DVStreaming = NULL;
> > >+  }
> > >+}
> > You could let the class destructor take care of destroying the m3DVStreaming
> > object through the nsRefPtr destructor
> 
> If I set m3DVStreaming to NULL as above, wouldn't that cause the object to get
> destroyed through the nsRefPtr destructor? Or are you suggesting that I can
> just move all this code from Uninitialize() into the 3DVUtils destructor? (I
> wanted to have an UnInitialize() to make sure that it pairs up with the
> Initialize(), which is where the 3D Streaming COM object initialization
> happens)

Having Uninitialize is fine! If you set it to NULL here it will get released through the nsRefPtr assignment operator. When the nsRefPtr is destroyed at the end of the Nv3DVUtils destructor it will find no pointer and do nothing. You could not set it to NULL here though and just let the nsRefPtr destructor handle it. But it doesn't matter much!

Comment 30

[Atul]

Attachment: Patch to enable html5 stereo rendering

Incorporated feedback from comment #26 into the patch.

Comment 31

[Bas Schouten (:bas.schouten)]

(In reply to comment #30)
> Created attachment 467889 [details] [diff] [review]
> Patch to enable html5 stereo rendering
> 
> Incorporated feedback from comment #26 into the patch.

I was going to land it but it doesn't seem to apply on current mozilla-central, could you update it so I can land it?

Comment 32

[Bas Schouten (:bas.schouten)]

(In reply to comment #31)
> (In reply to comment #30)
> > Created attachment 467889 [details] [diff] [review] [details]
> > Patch to enable html5 stereo rendering
> > 
> > Incorporated feedback from comment #26 into the patch.
> 
> I was going to land it but it doesn't seem to apply on current mozilla-central,
> could you update it so I can land it?

Actually, the rejects are easy enough I can figure them out I think, let me give it a shot.

Comment 33

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/mozilla-central/rev/e9992d1e029f

Comment 34

[Jeff Muizelaar [:jrmuizel]]

This change added the following warnings:

c:\users\jrmuizel\mozilla-central\obj-ff-profile\dist\include\nsAutoPtr.h(104) : warning C4150: deletion of pointer to incomplete type 'mozi
lla::layers::Nv3DVUtils'; no destructor called
        c:\users\jrmuizel\mozilla-central\gfx\layers\d3d9\DeviceManagerD3D9.h(52) : see declaration of 'mozilla::layers::Nv3DVUtils'
        c:\users\jrmuizel\mozilla-central\obj-ff-profile\dist\include\nsAutoPtr.h(103) : while compiling class template member function 'nsA
utoPtr<T>::~nsAutoPtr(void)'
        with
        [
            T=mozilla::layers::Nv3DVUtils
        ]
        c:\users\jrmuizel\mozilla-central\gfx\layers\d3d9\DeviceManagerD3D9.h(202) : see reference to class template instantiation 'nsAutoPt
r<T>' being compiled
        with
        [
            T=mozilla::layers::Nv3DVUtils
        ]
c:\users\jrmuizel\mozilla-central\obj-ff-profile\dist\include\nsAutoPtr.h(71) : warning C4150: deletion of pointer to incomplete type 'mozil
la::layers::Nv3DVUtils'; no destructor called
        c:\users\jrmuizel\mozilla-central\gfx\layers\d3d9\DeviceManagerD3D9.h(52) : see declaration of 'mozilla::layers::Nv3DVUtils'
        c:\users\jrmuizel\mozilla-central\obj-ff-profile\dist\include\nsAutoPtr.h(68) : while compiling class template member function 'void
 nsAutoPtr<T>::assign(T *)'
        with
        [
            T=mozilla::layers::Nv3DVUtils
        ]

Comment 35

[[e-mail]]

Comment on attachment 467889 [details] [diff] [review]
Patch to enable html5 stereo rendering

>+      nsString msg;
>+      msg += NS_LITERAL_STRING("CoCreateInstance() FAILED.\n");
>+      consoleCustom->LogStringMessage(msg.get());
I get squillions of these messages filling up the Error Console :-(

>+    /* 
>+     * Initialize COM 
>+     */ 
>+    CoInitialize(NULL); 
Windows widget code calls OleInitialize(), I hope there isn't some sort of race condition.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bas Schouten (:bas) from comment #33)
> http://hg.mozilla.org/mozilla-central/rev/e9992d1e029f

Why did this land without any reference to this bug?

Comment 37

[Bas Schouten (:bas.schouten)]

(In reply to Ms2ger from comment #36)
> (In reply to Bas Schouten (:bas) from comment #33)
> > http://hg.mozilla.org/mozilla-central/rev/e9992d1e029f
> 
> Why did this land without any reference to this bug?

Presumably because the patch author didn't include said reference. Whoever pushed it (presumably myself), didn't verify this.