507749 - Implement device url, protocol handler and camera channel and input stream

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: implements moz-device: protocol and camera channel+input stream

+++ This bug was initially created as a clone of Bug #451674 +++

In order to supply a live video preview for a camera input tag for content, we need to implement an input stream to supply the data to the video tag.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 392005 [details] [diff] [review]
implements moz-device: protocol and camera channel+input stream

Chris, could you have a look at the gstreamer related parts of this patch?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

If we want to standardize device: , Device APIs and Policy WG http://www.w3.org/2009/05/DeviceAPICharter.html might be the right place.

Comment 3

[Doug Turner (:dougt)]

i am not sure i like adding a generic url schema for devices.  What was wrong with camera:?

Comment 4

[Jeff Walden [:Waldo]]

A purely yak-shaving concern: assuming at some point we would want to standardize a protocol for accessing devices with the IETF or similar, it is probably simpler to give each type of device its own protocol so that each device's RFC can evolve separately, allowing people to separately watch and participate in work on each device as their proficiencies might suggest.  It's faster to evolve a specification for one device than it is to evolve one for many, particularly if every device is trying to get its own updates in concurrently.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 392005 [details] [diff] [review]
implements moz-device: protocol and camera channel+input stream

jduell suggested dougt as a reviewer

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 392005 [details] [diff] [review]
implements moz-device: protocol and camera channel+input stream

i think i would be happer with:

moz-camera:*


+{ /* 6b0ffe9e-d114-486b-aeb7-da62e7273ed5 */         \
+    0x60ffe9e,                                      \


Line up these \'s.  There are a bunch of whitespace oddities in the patch.  lets just say, clean them all up here so that I do not have to point them out everywhere in these review comments.


It is sort of wierd that the nsDeviceHandler is defined in the camera header.  We should either make this the "camera handler", or put stuff into a new header.


+    
+#ifdef NECKO_PROTOCOL_device
+    // from netwerk/protocol/device:
+    { NS_DEVICEHANDLER_CLASSNAME, 
+      NS_DEVICEHANDLER_CID,
+      NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "moz-device", 
+      nsDeviceHandlerConstructor
+    },
+#endif
 
     // from netwerk/protocol/about (about:blank is mandatory):
     { NS_ABOUTPROTOCOLHANDLER_CLASSNAME, 
@@ -1120,6 +1135,15 @@
     },
 #endif
 
+#ifdef NECKO_PROTOCOL_device
+    //moz-device:
+    { "The Device Protocol Handler", 
+      NS_DEVICEHANDLER_CID,
+      NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "moz-device",
+      nsDeviceHandlerConstructor
+    },
+#endif
+

Are you defining the protocal handler twice in the necko module list?



* Please document all of your IDLs.

diff -r b94bc4be53ca -r dd271eb774c7 netwerk/protocol/device/public/nsIDeviceURL.idl


why does this have to be native only?

	+  virtual nsresult GetEncoding(DeviceEncoding*) = 0;



nit: might be a good idea to change "init" to an attribute that be set, or maybe "setCameraStreamURL" or something more descriptive.



diff -r b94bc4be53ca -r dd271eb774c7 netwerk/protocol/device/src/nsCameraChannel.cpp


+// The n810's camera can only handle 8 fps, we should auto discover this though
+#ifdef MOZ_PLATFORM_HILDON
+#define FRAMERATE 8
+#else
+#define FRAMERATE 15
+#endif


Preference?  Why 15 for the default setting?


Before reviewing the IO part of this patch, why wasn't exists data structure implementation used, such as prclist, or segmented buffers?

* AllocateDataBuffer probably should zero out the various fields (buffer*, next, prev).


Does nsGstBufferHandlder should probably be in its own file, and it probably doesn't need to be a freind class of nsCameraInput.  Just put the required stuff in the nsBufferHandler base clase.

You should also document the base class a bunch so that when new "streamers" are easier to implement.



>+nsGstBufferHandler::~nsGstBufferHandler() {
>+   this->Shutdown();
>+}

There is a bit of an inconsistency.  It seems that you must 'new' a mBufferHandler and call Startup() on it, but you only have to delete mBufferHandler to delete and clean it up.  But in nsCameraInputStream::Close(), you do call shutdown explictly.



in ReadSegments:

>+  if (db)
>+    PR_Lock(db->lock);
>+  while (count > 0 && db) {

early return if db==null?


nsresult nsCameraChannel::Init(nsIURI* uri)

Should you have a default case (I know you are only have two types now).  If not, maybe switch around to only change the content type if the stream is OGG.


This pattern:

>+  if (mStats)
>+    *aHeight = mStats->height;
>+  else
>+    *aHeight = 0;

can be:

   aHeight = mStats ? mStats->height : 0;

Also add check your parms with NS_ENSURE_POINTER.


On the licenses, I think Nino is also a contributor on many of them, right?


>+  // Called to construct a blocking file input stream for the given file

What does this mean when you pass the async flag to OpenContentStream()


>+    if (!url) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }

no need for curly braces


any mochitests, or unit tests?

Also, as discussed, we need caps on the amount of memory that we stream to.  (consider the case when the consumer doesn't respond and we keep cacheing up video in memory)

I think the major things are:

1) change to use moz-camera.
2) change to use an existing data structure for memory management (or a good reason why this approach is better)
3) move all of the gstreaming stuff out to a new file and document that friend class so that (a) new streams can be implemented easier and (b) chris can verify your implementation correctness.

		and of course, all of the nits above.

Comment 7

[Doug Turner (:dougt)]

chris, do you also want to look at this patch?  its been sitting for a while, and I would really like to see something like this in ff 3.next

Comment 8

[cajbir (:cajbir)]

I believe the WHATWG has added a 'device' capability and they provide an example for webcams:

http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#devices

<p>To start chatting, select a video camera: <device type=media onchange="update(this.data)"></p>
<video autoplay></video>
<script>
 function update(stream) {
   document.getElementsByTagName('video')[0].src = stream.URL;
 }
</script>

How much work would it be to move towards an implementation like this?

This patch uses GStreamer - I'd really like to see it built on top of bug 422540 so as not to duplicate effort.

Comment 9

[cajbir (:cajbir)]

Comment on attachment 392005 [details] [diff] [review]
implements moz-device: protocol and camera channel+input stream

See comment 8 for details. Would like to be rebased on the GStreamer bug's patch. You might also like to get that bug's assignee to look at the GStreamer code to see how it fits in with what he's doing.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

updated to trunk, also switches from rgb to yuv as this seems preferable

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this implements reading in an oggyuv stream (as defined here: http://wiki.xiph.org/OggYUV).  It also gets rid of the nsICameraInputStream interface since it is not needed anymore.

Comment 12

[cajbir (:cajbir)]

How doe the GStreamer stuff in this patch relate to the code in bug 422540? What plans are there for making sure we don't end up with duplicate GStreamer support code? Any comment on my previous questions in comment 8 and 9?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

The only shared code between this and the other gstreamer bug is the configure code. It is taking a network stream and feeding it to gstreamr to render, while this is creating a network stream from a gstreamer source. I suppose we could abstract out constructing the graph, but I don't think we'd gain anything by it.

The whatwg spec you pointed out is kinda sorta like the w3c dap camera spec in that you get the stream via js calls. It shouldn't be hard to implement either, but I'd like to go with this DOM oriented approach for now and then look at a us api when one of those two mature.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 445473 [details] [diff] [review]
patch

sound like there is a better patch on the way.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Protocol Handler

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 463319 [details] [diff] [review]
Protocol Handler

Actually, this won't work in a disable-libxul build.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Protocol handler

Comment 18

[Josh Matthews [:jdm]]

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: let's make this tab-width: 8.

>+#ifndef _nsDeviceCaptureProvider_h__
>+#define _nsDeviceCaptureProvider_h__

Nit: get rid of the leading and trailing _s.

>+  virtual nsresult Init(nsACString& aContentType,
>+                        nsCaptureParams* aParams,
>+			nsIInputStream **aStream) = 0;

Nit: make all *s snuggle the type.

>+++ a/netwerk/protocol/device/nsDeviceChannel.cpp

>+void extractAttributeValue(const char *searchString, const char *attributeName, nsCString& result)

Nit: make the *s agree.

>+{
>+  result.Truncate();
>+
>+  if (searchString && attributeName) {
>+    PRUint32 attributeNameSize = strlen(attributeName);
>+    const char *startOfAttribute = PL_strcasestr(searchString, attributeName);
>+    if (startOfAttribute &&
>+        ( *(startOfAttribute-1) == '?' || *(startOfAttribute-1) == '&') ) {
>+      startOfAttribute += attributeNameSize; // Skip the attributeName
>+      if (*startOfAttribute) {
>+	const char *endOfAttribute = strchr(startOfAttribute, '&');
>+	if (endOfAttribute) {
>+	  result.Assign(Substring(startOfAttribute, endOfAttribute));
>+	} else {
>+	  result.Assign(startOfAttribute);
>+	}
>+      }
>+    }
>+  }
>+}

Nit: there's a mix of spaces and tabs in here.

However, more importantly, I really dislike the absence of early returns.  Could we rewrite it like so?

void
extractAttributeValue(const char *searchString, const char *attributeName, nsCString& result)
{
  result.Truncate();

  if (!searchString || attributeName)
    return;

  PRUint32 attributeNameSize = strlen(attributeName);
  const char *startOfAttribute = PL_strcasestr(searchString, attributeName);
  if (!startOfAttribute ||
      ( *(startOfAttribute-1) != '?' && *(startOfAttribute-1) != '&') )
    return;

  startOfAttribute += attributeNameSize; // Skip the attributeName
  if (!*startOfAttribute)
    return;
   
  const char *endOfAttribute = strchr(startOfAttribute, '&');
  if (endOfAttribute) {
    result.Assign(Substring(startOfAttribute, endOfAttribute));
  } else {
    result.Assign(startOfAttribute);
  }
}

I would also accept a followup to rewrite it in both places.

>+// nsDeviceChannel methods
>+nsDeviceChannel::nsDeviceChannel() {
>+  SetContentType(NS_LITERAL_CSTRING("image/png"));
>+}

Nit: { on the next line.

>+nsresult nsDeviceChannel::Init(nsIURI* aUri)
>+{
>+  nsBaseChannel::Init();
>+  nsBaseChannel::SetURI(aUri);
>+  return NS_OK;
>+}

Nit for almost every function declaration in this patch: the return type should stand alone on the previous line.

>+nsresult nsDeviceChannel::OpenContentStream(PRBool aAsync,
>+                                            nsIInputStream **aStream,
>+                                            nsIChannel **aChannel)
>+{
>+  if (!aAsync)
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  nsresult rv;

rv is unused.

>+  nsCOMPtr<nsIURI> uri = nsBaseChannel::URI();
>+  nsCAutoString spec;
>+  nsCAutoString type;
>+  nsCAutoString buffer;
>+  nsRefPtr<nsDeviceCaptureProvider> capture = nsnull;
>+  nsCaptureParams captureParams;
>+  PRUint32 err;
>+  *aStream = nsnull;
>+  *aChannel = nsnull;
>+  NS_NAMED_LITERAL_CSTRING(width, "width=");
>+  NS_NAMED_LITERAL_CSTRING(height, "height=");

Can we get these declared as they're used, instead of in this big block at the top?

>+  nsRefPtr<nsDeviceCaptureProvider> capture = nsnull;

Having conversed on IRC about how this function will always return NS_ERROR_FAILURE right now, I'm going to insist on a comment documenting this fact.

>+++ a/netwerk/protocol/device/nsDeviceChannel.h

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: tab-width: 8

>+#ifndef _NS_DEVICE_CHANNEL_h__
>+#define _NS_DEVICE_CHANNEL_h__

Nit: lower case, strip _s.

>+#include "imgIEncoder.h"
>+#include "mozilla/Mutex.h"
>+#include "nsBaseChannel.h"
>+#include "nsCOMPtr.h"
>+#include "nsDeque.h"
>+#include "nsIURI.h"
>+#include "nsNetCID.h"
>+#include "nsString.h"

I posit that many of these are not necessary.

>+++ a/netwerk/protocol/device/nsDeviceProtocolHandler.cpp

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: tab-width: 8

>+++ a/netwerk/protocol/device/nsDeviceProtocolHandler.cpp

>+  ~nsDeviceURI()
>+  {}

Nit: {} on the same line please.

>+  nsRefPtr<nsIURI> mSimpleURI;

This should either be a refptr to nsSimpleURI or an nsCOMPtr.

>+  nsDeviceURI* uri = new nsDeviceURI(baseURI);
>+
>+  nsresult rv = uri->Init(spec);
>+  NS_ENSURE_SUCCESS(rv, rv);

We'll leak if we bail out here.  nsRefPtr?

>+  nsRefPtr<nsDeviceChannel> channel = new nsDeviceChannel();
>+  nsresult rv = channel->Init(aURI);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  CallQueryInterface(channel, aResult);
>+
>+  return NS_OK;

Do we not care about the result of CallQueryInterface here?

>+++ a/netwerk/protocol/device/nsDeviceProtocolHandler.h

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: tab-width: 8

>+#ifndef nsDeviceProtocolHandler_h___
>+#define nsDeviceProtocolHandler_h___

Nit: Strip trailing _s

+  nsDeviceProtocolHandler()
+  {}
+  ~nsDeviceProtocolHandler()
+  {} 

Nit: Move the impls onto the same line.

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 463353 [details] [diff] [review]
Protocol handler

I'll give this r- for a couple important issues I'd like fixed up, but it's almost there.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch w/comments addressed

Comments addressed.  I'll file a followup on fixing the version of extractAttributeValue in imagelib.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 463590 [details] [diff] [review]
Patch w/comments addressed

With the caveat that

>+  nsDeviceURI(nsIURI* aURI)
>+    : mSimpleURI(new nsSimpleURI())

should be |new nsSimleURI(aURI)|, these changes look good to me. r=me

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 463590 [details] [diff] [review]
Patch w/comments addressed

Actually, we should be able to just use nsSimpleUri directly.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch w/nsSimpleUri

Like so.

We ignore the base URI parameter because a base URI doesn't make any sense for us.

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 464082 [details] [diff] [review]
Patch w/nsSimpleUri

>+    nsresult err;

This made its way back in again.  Kill it with fire.

Apart from that, I like this patch even better than the last.  r=me

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #24)
> Comment on attachment 464082 [details] [diff] [review]
> Patch w/nsSimpleUri
> 
> >+    nsresult err;
> 
> This made its way back in again.  Kill it with fire.
> 
> Apart from that, I like this patch even better than the last.  r=me

That's because it's necessary.  ToInteger has an nsresult outparam.  http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#373

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Try seems to approve.  Will push once I have the stamps.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/7fd64159634a

Comment 28

[Doug Turner (:dougt)]

kyle, can this be closed?