* 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?

As Benjamin's determination, this is a format issue (MP3), not a connect port problem.

(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.

Hi Ethan, no, we don't support MP3 over RTSP for now.

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.

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

(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.

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.

(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:".

Refresh commit message "r=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.

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.

The result of Try server: https://tbpl.mozilla.org/?tree=Try&rev=32c3f39dda85

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".

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

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).

(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.

(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?

Preeti: How locked down is the 1.4 branch? Is this small crash fix something you would want for 1.4?

Chris, I'll need risk analysis here. What's the user impact? If not significant I'd rather not take it.

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.

(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. :)