921963 - [RTSP] Support RTSP in Android JB build

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

The name of logging functions are different in ICS and Android JB. We should force the code to load our own logging header to prevent this problem. 

[Ref]: Bug 908797

Comment 1

[Ken Chang[:kenkai]]

We should support RTSP on JB.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0 for android Jelly beam platform

The log function name is different in Android ICS and Jelly Beam platform. 
Re-define them to prlog.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 831424 [details] [diff] [review]
Patch v1.0 for android Jelly beam platform

Review of attachment 831424 [details] [diff] [review]:
-----------------------------------------------------------------

Almost :) Logging changes look fine. But I'd rather that you changed the IPDL function params to use nsresult than doing casting. Maybe there is some reason I'm not aware of.

r- for now.

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +65,5 @@
>    nsresult rv = mController->AsyncOpen(this);
>    if (NS_SUCCEEDED(rv)) return true;
>  
>    mController = nullptr;
> +  return SendAsyncOpenFailed((uint8_t)rv);

I'd rather that you changed the IPDL to use an nsresult in the param list than do all this casting. See [Send|Recv]CancelHTMLDNSPrefetch for an example.

@@ +72,5 @@
>  bool
>  RtspControllerParent::RecvPlay()
>  {
>    LOG(("RtspControllerParent::RecvPlay()"));
> +  NS_ENSURE_TRUE(mController, true);

Do you want the error return value to be true here? Is the point just to flag the error on the console and return safely?

There are other statements like this in this file.

::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
@@ +13,5 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  
>  //#define LOG_NDEBUG 0

Is this used for anything? If it enables a particular kind of logging, can you add a comment please? Otherwise, please remove this line.

Please repeat for the other instances of this #define.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

Address the review comments.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 832877 [details] [diff] [review]
Patch v1.1

Review of attachment 832877 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for attending to the comments. You don't need to remove 'reason' from the log in RecvOnDisconnected (and other places), unless you really want to remove it.

r=me.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +142,2 @@
>  {
> +  LOG(("RtspControllerChild::RecvOnDisconnected %d", index));

You can still keep reason in the log, right? Just use 0x%x instead of %d.

Check other places.

@@ +248,5 @@
>  NS_IMETHODIMP
>  RtspControllerChild::Play(void)
>  {
>    LOG(("RtspControllerChild::Play()"));
> +  NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);

Good catch. I've done this myself several times :)

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Try server result,
https://tbpl.mozilla.org/?tree=Try&rev=c5f7c131c1d1

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2

Update the patch to address the review comments.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0def2d6e3665

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0def2d6e3665