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

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: blassey, Assigned: khuey)

Tracking

(Blocks: 2 bugs, {mobile})

unspecified
All
Linux
mobile
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Created attachment 392005 [details] [diff] [review]
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.
Attachment #392005 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

8 years ago
Blocks: 451674
No longer depends on: 451674
(Reporter)

Updated

8 years ago
Attachment #392005 - Flags: review?(chris.double)
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

8 years ago
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

8 years ago
i am not sure i like adding a generic url schema for devices.  What was wrong with camera:?
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.
Attachment #392005 - Flags: review?(jduell.mcbugs)
Comment on attachment 392005 [details] [diff] [review]
implements moz-device: protocol and camera channel+input stream

jduell suggested dougt as a reviewer
Attachment #392005 - Flags: review?(doug.turner)

Comment 6

8 years ago
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.
Attachment #392005 - Flags: review?(doug.turner) → review-
Blocks: 511387

Comment 7

8 years ago
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

8 years ago
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

7 years ago
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.
Attachment #392005 - Flags: review?(chris.double) → review-
Created attachment 445417 [details] [diff] [review]
patch

updated to trunk, also switches from rgb to yuv as this seems preferable
Assignee: nobody → blassey.bugs
Attachment #392005 - Attachment is obsolete: true
Created attachment 445473 [details] [diff] [review]
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.
Attachment #445417 - Attachment is obsolete: true
Attachment #445473 - Flags: review?(dougt)
Attachment #445473 - Flags: review?(chris.double)

Comment 12

7 years ago
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?
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 on attachment 445473 [details] [diff] [review]
patch

sound like there is a better patch on the way.
Attachment #445473 - Flags: review?(dougt)
Attachment #445473 - Flags: review?(chris.double)
Created attachment 463319 [details] [diff] [review]
Protocol Handler
Assignee: blassey.bugs → me
Attachment #445473 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #463319 - Flags: review?(josh)
Comment on attachment 463319 [details] [diff] [review]
Protocol Handler

Actually, this won't work in a disable-libxul build.
Attachment #463319 - Attachment is obsolete: true
Attachment #463319 - Flags: review?(josh)
Created attachment 463353 [details] [diff] [review]
Protocol handler
Attachment #463353 - Flags: review?(josh)
>+/* -*- 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 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.
Attachment #463353 - Flags: review?(josh) → review-
Created attachment 463590 [details] [diff] [review]
Patch w/comments addressed

Comments addressed.  I'll file a followup on fixing the version of extractAttributeValue in imagelib.
Attachment #463353 - Attachment is obsolete: true
Attachment #463590 - Flags: review?(josh)
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
Attachment #463590 - Flags: review?(josh) → review+
Comment on attachment 463590 [details] [diff] [review]
Patch w/comments addressed

Actually, we should be able to just use nsSimpleUri directly.
Attachment #463590 - Attachment is obsolete: true
Created attachment 464082 [details] [diff] [review]
Patch w/nsSimpleUri

Like so.

We ignore the base URI parameter because a base URI doesn't make any sense for us.
Attachment #464082 - Flags: review?(josh)
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
Attachment #464082 - Flags: review?(josh) → review+
(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
Depends on: 587530
tracking-fennec: --- → ?
Try seems to approve.  Will push once I have the stamps.
(Reporter)

Updated

7 years ago
tracking-fennec: ? → 2.0+
http://hg.mozilla.org/mozilla-central/rev/7fd64159634a
kyle, can this be closed?
Status: ASSIGNED → NEW
tracking-fennec: 2.0+ → 2.0-

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

6 years ago
No longer blocks: 451674

Updated

6 years ago
Blocks: 692955
You need to log in before you can comment on or make changes to this bug.