Closed Bug 962309 Opened 9 years ago Closed 9 years ago

Video RTSP support should be disabled in 1.3

Categories

(Core :: Audio/Video, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jsmith, Assigned: ethan)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(1 file, 1 obsolete file)

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.
blocking-b2g: --- → 1.3?
Video RTSP work and related bugs are tracked/triaged by Wesley_Huang
Flags: needinfo?(whuang)
Flags: needinfo?(skasetti)
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?
Flags: needinfo?(whuang)
(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.
Flags: needinfo?(skasetti) → needinfo?(mkhoo)
Discussed offline - we aren't going to do this. We'll instead be moving forward with stabilizing video support with audio support for RTSP.
Status: NEW → RESOLVED
blocking-b2g: 1.3? → ---
Closed: 9 years ago
Flags: needinfo?(mkhoo)
Resolution: --- → WONTFIX
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.
Blocks: b2g-RTSP-1.3
Status: RESOLVED → REOPENED
blocking-b2g: --- → 1.3+
Resolution: WONTFIX → ---
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
Flags: needinfo?(ettseng)
Whiteboard: [FT:RIL]
(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).
Flags: needinfo?(ettseng)
Assignee: nobody → ettseng
Attached patch bug-962309-fix.patch (obsolete) — Splinter Review
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?
Attachment #8367164 - Flags: review?(sworkman)
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core
Version: unspecified → 28 Branch
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.
Attachment #8367164 - Flags: review?(sworkman) → review+
Fixed the issues according to the review comment.
Attachment #8367723 - Flags: review+
Attachment #8367164 - Attachment is obsolete: true
(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.
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 :)
(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. :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea29845aa123
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This shouldn't land on trunk - can you back this out of m-c?
Flags: needinfo?(ryanvm)
Comment 13 says otherwise?
Flags: needinfo?(ryanvm)
(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.
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
You need to log in before you can comment on or make changes to this bug.