Closed Bug 1009497 Opened 7 years ago Closed 7 years ago

[RTSP][V2.0] Crash happened while device plays MP3 stream over RTSP

Categories

(Firefox OS Graveyard :: RTSP, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, b2g-v1.3T wontfix, b2g-v1.4 wontfix, b2g-v2.0 fixed)

VERIFIED FIXED
2.0 S3 (6june)
blocking-b2g 2.0+
Tracking Status
b2g-v1.3T --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed

People

(Reporter: whsu, Assigned: ethan)

References

Details

(Keywords: crash, Whiteboard: [b2g-crash] [p=2])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached video WP_20140513_003.mp4
* Description:
  The crash happened on RTSP streaming.
  While device connects to specific port to play RTSP streaming, system crash happened.
  Here are the detailed information:
   - Title:
     ~ B2G 32.0a1 Crash Report [@ __libc_android_abort | __android_log_assert | android::ASessionDescription::getFormatType(unsigned int, unsigned long*, android::AString*, android::AString*) const ])

   - Crash Report:
     ~ https://crash-stats.mozilla.com/report/index/1bfbd0ab-a876-42a0-85a2-6a7b42140513

   - Demo video:
     ~ WP_20140513_003.mp4

* Reproduction steps:
  1. Launch browser app
  2. Input following link on URL bar then tap "Enter" key
     - rtsp://10.247.30.190:8000/rtsp.mp3

* Expected result:
  Built-in media player plays RTSP streaming

* Actual result:
  A system crash happened

* Reproduction build: V2.0 (Buri)
 - Gaia      2f89c43e798ccba631025bedc47a1fb24e830cf2
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/4b6d63b05a0a
 - BuildID   20140512160204
 - Version   32.0a1
(In reply to William Hsu [:whsu] from comment #0)

> * Reproduction steps:
>   1. Launch browser app
>   2. Input following link on URL bar then tap "Enter" key
>      - rtsp://10.247.30.190:8000/rtsp.mp3

Hmm, for now, we don't support mp3 in RTSP.
So a quick fix that we should return an error notify to the user instead of crash
Hi Benjamin, 
Could you do the error handling for this one? 
Do we want to support mp3 in rtsp?
(In reply to Randy Lin [:rlin] from comment #2)
> Do we want to support mp3 in rtsp?

I was never aware of this issue until now!

We reuse the RTSP framework of Android's StageFright.
And a Wowza documentation also mentioned:
"Android devices can't play MP3 streams over RTSP/RTP in any combination (audio/video or audio only). Android devices that support Flash player 10.1 can play MP3 using RTMP or Adobe HDS."
http://www.wowza.com/forums/content.php?62

I don't know if it makes sense to support MP3 in our RTSP.
It seems to be a requirement level issue. How about consulting PM to make this decision?

Of course, no matter we will support it or not, we must avoid the crash first.
Some references:

- Wowza .mp3 RTSP stream on android (discussion on StackOverflow)
  http://stackoverflow.com/questions/14280865/wowza-mp3-rtsp-stream-on-android
- Supported Media Formats on Android
  http://developer.android.com/guide/appendix/media-formats.html
- How to troubleshoot RTSP/RTP playback on Wowza
  http://www.wowza.com/forums/content.php?62
Howie, should we support MP3 over our RTSP/RTP?
Could you help to clarify this requirement?
Flags: needinfo?(hochang)
As Benjamin's determination, this is a format issue (MP3), not a connect port problem.
Summary: [RTSP][V2.0] Crash happened while device connects specific port → [RTSP][V2.0] Crash happened while device plays MP3 stream over RTSP
(In reply to Ethan Tseng [:ethan] from comment #5)
> Howie, should we support MP3 over our RTSP/RTP?

*** Additional Remark ***
From the protocol perspective of RTSP/RTP, it should be viable to transport MP3.
However so far I don't know how much effort will be to add this support to our repository.
Crash Signature: [@ __libc_android_abort | __android_log_assert | android::ASessionDescription::getFormatType(unsigned int, unsigned long*, android::AString*, android::AString*) const ]
Keywords: crash
Whiteboard: [b2g-crash]
Hi Ethan, no, we don't support MP3 over RTSP for now.
Flags: needinfo?(hochang)
William,
Please help to verify that if this crash happens on a release build.
We might decide the priority of this bug depending on this result.
Flags: needinfo?(whsu)
Duplicate of this bug: 1009925
Hi, Ethan,

Sorry for the late reply.
I still can reproduce this bug on Tarako production build.
We need to fix this bug.
Attach the demo video. (WP_20140519_001.mp4)

* Build information:
 - Gaia      746934a1e331b9ce578bd9fbdb4614d950baf765
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/771fedd3e904
 - BuildID   20140506164003
 - Version   28.1
Flags: needinfo?(whsu)
Attached video WP_20140519_001.mp4
(In reply to William Hsu [:whsu] from comment #11)
> Hi, Ethan,
> Sorry for the late reply.
> I still can reproduce this bug on Tarako production build.
> We need to fix this bug.

Thanks for you verification.
Apparently we must raise the priority of this bug.
I will devote my attention to it quite soon.

p.s. Those CHECK() assertions are still executed in a release/production build, which is against our assumption.
Assignee: nobody → ettseng
Severity: normal → critical
Priority: -- → P1
Blocks: b2g-RTSP-2.0
Status: NEW → ASSIGNED
Add error handle to ASessionDescription::getFormatType() to avoid crash.
1. Return bool instead of void to represent success/failure.
2. Replace the original assertion of findAttribute() in getFormatType() by a conditional check.
Hi, Steve

I would like to request your review.
I will explain what we are dealing with here.

The crash happens in the assertion in line 216 of ASessionDescription.cpp. This is performed when parsing the RTSP/SDP response to an RTSP DESCRIBE request.
214     sprintf(key, "a=rtpmap:%lu", x);
216     CHECK(findAttribute(index, key, desc));

We can tell from these two lines that "a=rtpmap:" line MUST exist in the SDP packet.
Now let me briefly explain the purpose of "a=rtpmap:".

RFC 4566 (http://tools.ietf.org/html/rfc4566.html) provides an example SDP description:
      v=0
      o=jdoe 2890844526 2890842807 IN IP4 10.47.16.5
      s=SDP Seminar
      i=A Seminar on the session description protocol
      u=http://www.example.com/seminars/sdp.pdf
      e=j.doe@example.com (Jane Doe)
      c=IN IP4 224.2.17.12/127
      t=2873397496 2873404696
      a=recvonly
      m=audio 49170 RTP/AVP 0
      m=video 51372 RTP/AVP 99
      a=rtpmap:99 h263-1998/90000

The "a=rtpmap:" line makes a dynamic payload type assignment.
In the example above, the payload type for audio is 0, which is a static assignment in the profile, the payload format for AUDIO/PCMU.
The payload type for video is 99, which is mapped to the payload format for video/h263-1998 by the "a=rtpmap:" line.

Additional remarks:
1. RTP audio video profile: http://en.wikipedia.org/wiki/RTP_audio_video_profile
2. Payload types in the range 96 to 127 are reserved for dynamic assignment.


When we reproduce this bug by playing an MP3 audio from a live555 media server, the SDP description doesn't contain an "a=rtpmap:" line.
I guess it is because the MP3 format is static (MPEG-I/II audio), and the server implementation just ignores "a=rtpmap:" since no dynamic payload type assignment is needed for this media.

In conclusion, our Android-based RTSP client does not support SDP without dynamic payload type assignment for now.
I just tweak the code to replace the assertion by an error handling.
Attachment #8426923 - Flags: feedback?(sworkman)
(In reply to Ethan Tseng [:ethan] from comment #15)

Thanks for the explanation!

> When we reproduce this bug by playing an MP3 audio from a live555 media
> server, the SDP description doesn't contain an "a=rtpmap:" line.
> I guess it is because the MP3 format is static (MPEG-I/II audio), and the
> server implementation just ignores "a=rtpmap:" since no dynamic payload type
> assignment is needed for this media.

Right - I read some of the links you sent, and it seems like "a=rtpmap:" is only needed if the "m=" line contains dynamic IDs. So, it would be good to add to your patch the ability to detect if "a=rtpmap:" is needed or not. Then, if it is needed but not present, the function can return false/error.

> In conclusion, our Android-based RTSP client does not support SDP without
> dynamic payload type assignment for now.
> I just tweak the code to replace the assertion by an error handling.

Sounds good. Let's land that now, since this is high priority. Maybe you can file a follow-up bug if you think that we could add some more conditions around "a=rtpmap:".
Attachment #8426923 - Flags: feedback?(sworkman) → feedback+
Whiteboard: [b2g-crash] → [b2g-crash] [p=2]
Target Milestone: --- → 2.0 S3 (6june)
Refresh commit message "r=sworkman".
Attachment #8426923 - Attachment is obsolete: true
Attachment #8427487 - Flags: review?(sworkman)
(In reply to Steve Workman [:sworkman] from comment #16)
> 
> Sounds good. Let's land that now, since this is high priority. Maybe you can
> file a follow-up bug if you think that we could add some more conditions
> around "a=rtpmap:".

Steve, I deemed your comment as approval of the patch so I refreshed its commit message.
But to ensure I didn't misunderstand, I still raised the "review?" flag to ask for your confirmation.

I checked relevant codes about findAttribute() and getFormatType() again. I think no more to be added to the patch for now.
Add some code comments to make it more clear.
Attachment #8427487 - Attachment is obsolete: true
Attachment #8427487 - Flags: review?(sworkman)
Attachment #8427491 - Flags: review?(sworkman)
blocking-b2g: --- → 2.0?
Comment on attachment 8427491 [details] [diff] [review]
Add error handle for unsupported format type

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

(In reply to Ethan Tseng [:ethan] from comment #18)
> Steve, I deemed your comment as approval of the patch so I refreshed its
> commit message.
> But to ensure I didn't misunderstand, I still raised the "review?" flag to
> ask for your confirmation.

Yup - if you're ready for review, then approved! r=me.

> I checked relevant codes about findAttribute() and getFormatType() again. I
> think no more to be added to the patch for now.

Okie doke.
Attachment #8427491 - Flags: review?(sworkman) → review+
https://hg.mozilla.org/mozilla-central/rev/1cffd9cf3f63
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Just for the record, this bug fixes the crash but doesn't add support to MP3-over-RTSP.

When the user tries to play MP3 (or any other unsupported media formats) over RTSP, an error message would be prompted, such as "video format error".
blocking-b2g: 2.0? → 2.0+
Thanks for the help!
It works but a minor bug still needs your help. (Bug 1021025)

* Build information: (V2.0 - Flame)
  - Gaia      a38a6a5c6fabc97dd16d5360632b5ac5c7e06241
  - Gecko     https://hg.mozilla.org/mozilla-central/rev/951e3a671279
  - BuildID   20140604160202
  - Version   32.0a1
Status: RESOLVED → VERIFIED
Ethan: is this RTSP crash fix relevant for B2G 1.4? This bug's status flags say it was fixed on B2G 2.0, but the bug was also reproducible on Tarako (comment 11).
status-b2g-v1.4: --- → ?
Flags: needinfo?(ettseng)
(In reply to Chris Peterson (:cpeterson) from comment #26)
> Ethan: is this RTSP crash fix relevant for B2G 1.4? This bug's status flags
> say it was fixed on B2G 2.0, but the bug was also reproducible on Tarako
> (comment 11).

Hi Chris,
Yes, this bug exists in all versions and was only fixed on B2G 2.0.
If needed, we can uplift the fix to both 1.3 and 1.4.
Flags: needinfo?(ettseng)
(In reply to Ethan Tseng [:ethan] from comment #27)
> (In reply to Chris Peterson (:cpeterson) from comment #26)
> > Ethan: is this RTSP crash fix relevant for B2G 1.4? This bug's status flags
> > say it was fixed on B2G 2.0, but the bug was also reproducible on Tarako
> > (comment 11).
> 
> Hi Chris,
> Yes, this bug exists in all versions and was only fixed on B2G 2.0.
> If needed, we can uplift the fix to both 1.3 and 1.4.

It's probably too late for 1.3. Not sure about 1.4 however.
(In reply to Jason Smith [:jsmith] from comment #28)
> (In reply to Ethan Tseng [:ethan] from comment #27)
> > Hi Chris,
> > Yes, this bug exists in all versions and was only fixed on B2G 2.0.
> > If needed, we can uplift the fix to both 1.3 and 1.4.
> 
> It's probably too late for 1.3. Not sure about 1.4 however.

So should we try to uplift to 1.4?
Flags: needinfo?(cpeterson)
Preeti: How locked down is the 1.4 branch? Is this small crash fix something you would want for 1.4?
Flags: needinfo?(cpeterson) → needinfo?(praghunath)
Chris,

I'll need risk analysis here. 

What's the user impact? If not significant I'd rather not take it.
Flags: needinfo?(praghunath) → needinfo?(cpeterson)
Rereading the bug description, we probably don't need to uplift to 1.4 because MP3 over RTSP is not a common use case, in part, because Android doesn't support it either.
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson (:cpeterson) from comment #32)
> Rereading the bug description, we probably don't need to uplift to 1.4
> because MP3 over RTSP is not a common use case, in part, because Android
> doesn't support it either.

Agreed. Thanks for your time to analyze this and make decision. :)
You need to log in before you can comment on or make changes to this bug.