962309 - Video RTSP support should be disabled in 1.3

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jason Smith [:jsmith]]

Video RTSP support has been marked out of scope for 1.3, but we still have the feature partially implemented on 1.3. However, there are open bugs like bug 951188 that showcase evidence that the feature is half-baked right now. We should disable the feature entirely on 1.3 to ensure that users do not get exposed to a broken feature.

Comment 1

[Hema Koka [:hema]]

Video RTSP work and related bugs are tracked/triaged by Wesley_Huang

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Jason,
I think we have critical ones resolved in 1.3.
For bug 951188, my opinion is to defer it to 1.4 for it's and experience enhancement. any comment?

Comment 3

[Jason Smith [:jsmith]]

(In reply to Wesley Huang [:wesley_huang] from comment #2)
> Hi Jason,
> I think we have critical ones resolved in 1.3.
> For bug 951188, my opinion is to defer it to 1.4 for it's and experience
> enhancement. any comment?

We already know bug 951188 is deferred to 1.4. However, deferring that argues for turning the feature off in 1.3. The bug & it's dupe showcase that a basic error flow with video RTSP generates a poor UX, as the video app loads with no end in site when an error is hit parsing a RTSP URL. I wouldn't be surprised if there were other problems with video RTSP present on 1.3 right now, given that we haven't fixing bugs on that branch.

The general rule is - if you defer a feature to a followup release, then you need to turn it off in the current release. If you don't do that, then you need to fix the bugs in 1.3. If we are diligent about turning half-baked features off, then we risk shipping a broken experience to our users.

Comment 4

[Jason Smith [:jsmith]]

Discussed offline - we aren't going to do this. We'll instead be moving forward with stabilizing video support with audio support for RTSP.

Comment 5

[Jason Smith [:jsmith]]

We discussed this further in triage again - Sandip advises that we throw an error when we try to playback a video from RTSP, given that the current quality state of RTSP video is poor. Reopening for implementing this.

Comment 6

[Hema Koka [:hema]]

Ethan Tseng, since you implemented the video RTSP feature, would be great if you can take care of throwing error when playing video from rtsp (per comment 5)

CC'ing Johu and djf

Comment 7

[Ethan Tseng [:ethan]]

(In reply to Hema Koka [:hema] from comment #6)
> Ethan Tseng, since you implemented the video RTSP feature, would be great if
> you can take care of throwing error when playing video from rtsp (per
> comment 5)
> CC'ing Johu and djf
Hi Hema,
We think it is doable to do this. I am working to generate a patch for this feature (throwing an error when playing video from RTSP).

Comment 8

[Ethan Tseng [:ethan]]

Attachment: bug-962309-fix.patch

What I did in this patch:
1. Add a preference "media.rtsp.video.enabled" to decide the RTSP video feature is on/off. The default value is off.
2. If RTSP video feature is off and the RTSP media being loaded is video, abort the loading and report an error to media element.

Hi Steve, could you please review this patch?

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8367164 [details] [diff] [review]
bug-962309-fix.patch

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

This looks fine to me. r=me with some really minor changes.

::: content/media/RtspMediaResource.cpp
@@ +454,5 @@
> +  for (int i = 0; i < tracks; ++i) {
> +    nsCOMPtr<nsIStreamingProtocolMetaData> trackMeta;
> +    mMediaStreamController->GetTrackMetaData(i, getter_AddRefs(trackMeta));
> +    MOZ_ASSERT(trackMeta);
> +    uint32_t w, h = 0;

w = 0, h = 0;

@@ +478,5 @@
>    }
>  
>    uint8_t tracks;
>    mMediaStreamController->GetTotalTracks(&tracks);
> +  // Bug 962309 - Video RTSP support should be disabled in 1.3

I don't think you need this comment. It might be useful in the future to always have this code, so it's probably better if we don't have a comment that mentions a specific bug or release. The revision history will provide enough pointers to more information for people that want to know.

Comment 10

[Ethan Tseng [:ethan]]

Attachment: bug-962309-fix.patch

Fixed the issues according to the review comment.

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #9)
> Comment on attachment 8367164 [details] [diff] [review]
> > +  // Bug 962309 - Video RTSP support should be disabled in 1.3
> I don't think you need this comment. It might be useful in the future to
> always have this code, so it's probably better if we don't have a comment
> that mentions a specific bug or release. The revision history will provide
> enough pointers to more information for people that want to know.
Steve, so do you think we should land this patch to both central and v1.3?
Originally I thought this should only be landed to v1.3.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Ah, I see. I don't mind either way. If you only want it to be on 1.3, you should keep the comment as is. But if you think it could be useful on central to have that extra pref, then a more generic comment, or none is better.

Most importantly, the code is fine. It's your choice with the comment :)

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #12)
> Ah, I see. I don't mind either way. If you only want it to be on 1.3, you
> should keep the comment as is. But if you think it could be useful on
> central to have that extra pref, then a more generic comment, or none is
> better.
> Most importantly, the code is fine. It's your choice with the comment :)
Good advice! Then I think it's better to keep central and v1.3 synchronized.
Thank you, Steve. :)

Comment 14

[Ethan Tseng [:ethan]]

The result of try server:
https://tbpl.mozilla.org/?tree=Try&rev=600c0da607cb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea29845aa123

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbda34620583

IIUC, this patch disabled RTSP on trunk (v1.4) as well?

Comment 18

[Jason Smith [:jsmith]]

This shouldn't land on trunk - can you back this out of m-c?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment 13 says otherwise?

Comment 20

[Ethan Tseng [:ethan]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #17)
> IIUC, this patch disabled RTSP on trunk (v1.4) as well?
Yes, that's what we meant.

Jason, originally I thought this patch shouldn't land on trunk. But since Steve suggests this patch (preference of RTSP video) might be useful in the future. I think it's better to keep them synchronized.
We can enable this default setting in another v1.4 RTSP bug or just file a new bug to do it. I think this is more clear for our codes.

Comment 21

[Ethan Tseng [:ethan]]

I just filed bug 966623 to turn on RTSP video in the default preference. It shall be landed on trunk so that we can keep moving forward to resolve RTSP video issues for v1.4