Steps to reproduce: ---------------------------------------------------------------------- 1. Copy attached music file to device 2. Launch Music app 3. Find the music copied in step 1 and play 4. Tap on the Share button and select "Set as Ringtone" 5. Make a MT call to device --> Ringtone could not play out. [NOTE] 1. This only happens to some amr files. Test build --------------------------------------------------------------------------- Build ID: 20140401060054 Gecko: 321494b801617a6bdf8260a483a80c0c09d49c4d Gaia: 769c3b00a43f03ca901414ec533f7b313a7684c5 Gonk: 575204d5d39f8b9329fb47a80db616c2362bf488

attached adb log. Test starts: 04-02 14:27:24.270

still need to check what's the problem of "https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/dialer_ringer.js#L156" I simply move "window.clearInterval(this._vibrateInterval);" to the line before "player.pause();" to avoid the issue.

I use the newest m-c version and roll back gaia to below version. It works normally. Gaia f1f99c6f880d2adbf5886a8dbe6345141c2d5626 Gecko f1b5b0594b9e6f76fe808070bdaac9e748e7b5ca BuildID 20140402103257 Version 31.0a1 The different between this 2 version is the js code related to ringtone is moved from communication\dailer\js\calls_handler.js to \system\js\dialer_ringer.js Hi, Etienne Do you have any comment or any advice? Thanks

Can you see if the patch on bug 983626 helps?

ni? James for comment 6

(In reply to Joe Cheng [:jcheng] from comment #7) > ni? James for comment 6 Jesse, please review.

i merged the patch into v1.3 ,but this issue is still can reproduced.

(In reply to Ming from comment #9) > i merged the patch into v1.3 ,but this issue is still can reproduced. Then we should hand this over to the WebAudio folks. Ming, can you attach the audio file in question? Should be helpful.

to Bruce Sun as he's looking at this. let's wait for further feedback from Bruce

Are we supposed to be able to decode .amr files on b2g? Certainly not in content, but maybe in apps? Also, this is not using Web Audio, but a regular <audio> element.

Update: MediaExtractor::countTracks() return 0 in OmxDecoder::Init(), so that OmxDecoder::Init() returns false.

triage: 1.3T+ POVB, to James for now. thanks

Loop Ming.li

Ok, i am working at this bug .

While playing the amr ringer from system app, nsFileInputStream::Read() will continuously be called before playing the ringer. And after that, nsFileInputStream::Close() will be called in nsFileInputStream::Read() right after the end-of-stream has been encountered: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.cpp#479 I've tried one tricky experiment. If we don't invoke Close() at this line, the ringer can be played and be heard. Comparing to music app, although CLOSE_ON_EOF flag is also set, but nsFileInputStream::Close() has never be called during the whole playback in original workflow. Not sure whether it matters, but the native path of the same file seems different in these two apps: - system app: "/data/local/storage/persistent/chrome/idb/2588645841ssegtnti/4" - music app: "/mnt/sdcard/_AMR_NB.amr" It is weird that mp3 files can be played in system app but amr files cannot be played. The only difference I can imagine is AMRExtractor might need to parse all contents of one amr file before playing it, but MP3Extractor can just use the beginning part of one mp3 file for necessary information. However, both extractors keep their behaviors no matter in system app or in music app. :bent, do we need to do some extra handling in order to play a file in the system app like this?

As discussed with Ming on phone, taking this bug from him.

When can we land this patch.

James, the patch is not reviewed yet, and it's unclear to me if Bruce is working on another solution.

I don't think I have enough information yet to guess what's going wrong here. So which process are we supposed to be playing the ringtone from? Is it the parent (system) process or the child (dialer) process? One major difference between parent and child processes is that the parent can close and reopen any files it wants while child processes cannot. I don't know if that is relevant but based on comment 17 perhaps it is. And it sounds like the way ringtones work is we set a file blob as a setting? Which would turn it into an indexedDB file eventually. Then we hopefully read from indexedDB when we try to play?

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #21) > I don't think I have enough information yet to guess what's going wrong here. > > So which process are we supposed to be playing the ringtone from? Is it the > parent (system) process or the child (dialer) process? One major difference > between parent and child processes is that the parent can close and reopen > any files it wants while child processes cannot. I don't know if that is > relevant but based on comment 17 perhaps it is. > Current ringtone is played from parent (system) process, and we encounter this bug in this circumstance. > And it sounds like the way ringtones work is we set a file blob as a > setting? Which would turn it into an indexedDB file eventually. Then we > hopefully read from indexedDB when we try to play? Dominic, would you please share your comments on this?

change ni from Dominic to Etienne due to bug 990003.

(In reply to Bruce Sun [:brsun] from comment #22) > (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #21) > > I don't think I have enough information yet to guess what's going wrong here. > > > > So which process are we supposed to be playing the ringtone from? Is it the > > parent (system) process or the child (dialer) process? One major difference > > between parent and child processes is that the parent can close and reopen > > any files it wants while child processes cannot. I don't know if that is > > relevant but based on comment 17 perhaps it is. > > > > Current ringtone is played from parent (system) process, and we encounter > this bug in this circumstance. After bug 990003 landed the ringtone still plays in the system app. > > And it sounds like the way ringtones work is we set a file blob as a > > setting? Which would turn it into an indexedDB file eventually. Then we > > hopefully read from indexedDB when we try to play? > > Dominic, would you please share your comments on this? Ben is correct. There is a app called "setringtone" in gaia, after setringtone app received the ringtones blob from music app via share activity, it saved the blob in mozSettings so that the ringtone can be accessible for either system, communication or settings. A possible workaround could be, additionally save the ringtone's path in mozSettings(currently we only save the ringtones's blob and name), and use it in system app to retrieve the ringtone from the deviceStorage api then play it, that should work because it's how music app plays the amr files, but if we need this, please, only for 1.3T or write XXX comments and remove it in the future.

quick update: this issue does NOT reproduce on gecko:1.3 / gaia:1.3

Just tried gecko:1.3T / gaia:1.3T on my Unagi device. It worked as expected. The amr ringtone could be played correctly.

When playing one AMR file, all data in the file will be parsed when MediaExtractor::Create() is called: http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#122 When playing one AMR file in the content process, nsIFileInputStream::CLOSE_ON_EOF will be cleared in nsFileInputStream::Serialize(): http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.cpp#559 When playing on AMR file in the parent process, nsMultiplexInputStream::Seek() will fail after all data in the file has been parsed and closed: http://dxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder.cpp#223 http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp#345

This is one possible solution: - Remove nsIFileInputStream::CLOSE_ON_EOF flag from sFileStreamFlags. - Note: All files created in the parent process will not be closed on EOF by default anymore.

This is another solution: - Explicitly call nsISeekableStream::Seek() when nsFileInputStream::Tell() or nsFileInputStream::Available() return NS_BASE_STREAM_CLOSED in nsMultiplexInputStream::Available(), nsMultiplexInputStream::Seek(), and nsMultiplexInputStream::Tell(). - Note: Implementation-specific behaviour of nsIInputStream or nsISeekableStream will be explicitly handled in nsMultiplexInputStream.

Each one of attachment 8408912 [details] [diff] [review] and attachment 8408914 [details] [diff] [review] can solve this issue. Need further comments from experts to distinguish which one is better or more suitable.

Bruce, let's move this to the right components.

This is also another solution: - Call nsFileInputStream::Seek() by nsFileInputStream itself when nsFileStreamBase::Tell() and nsFileStreamBase::Available() return NS_BASE_STREAM_CLOSED. - This one might be better than attachment 8408912 [details] [diff] [review] and attachment 8408914 [details] [diff] [review] since this patch sync the behaviour of mBehaviorFlags inside nsFileInputStream instead of workaround the behaviour of nsIFileInputStream::CLOSE_ON_EOF on callers.

We'd better land this patch before 4.25 -- spreadtrum PTR2 test cycle.

(In reply to Eric Chou [:ericchou] [:echou] from comment #26) > Just tried gecko:1.3T / gaia:1.3T on my Unagi device. It worked as expected. > The amr ringtone could be played correctly. interesting. I'd like someone to investigate the difference between Eric's unagi and a tarako here.

hi Bruce/Ben, can you please advise what's next? thanks

(In reply to Fabrice Desré [:fabrice] from comment #34) > (In reply to Eric Chou [:ericchou] [:echou] from comment #26) > > Just tried gecko:1.3T / gaia:1.3T on my Unagi device. It worked as expected. > > The amr ringtone could be played correctly. > > interesting. I'd like someone to investigate the difference between Eric's > unagi and a tarako here. I've tested the unagi device from Eric, and there should be some vender-specific implementation on extractors and registered sniffers. - AMR ringtone on unagi: The opened AMR file is not closed because EOF is not encountered while calling MediaExtractor::Create(). - AMR ringtone on tarako: The opened AMR file will be closed because EOF is encountered while calling MediaExtractor::Create().

(In reply to Joe Cheng [:jcheng] from comment #35) > hi Bruce/Ben, can you please advise what's next? thanks Hi Ben, what do you think of these patches? - attachment 8408912 [details] [diff] [review] - attachment 8408914 [details] [diff] [review] - attachment 8409521 [details] [diff] [review]

(In reply to Bruce Sun [:brsun] from comment #36) > - AMR ringtone on tarako: The opened AMR file will be closed because EOF is > encountered while calling MediaExtractor::Create(). One temporary solution on this bug is to avoid reading through EOF. If we already know in advance that the file size is 16360 (take attachment 8400441 [details] for example), it is not so reasonable to read with an offset equal to 16360 or larger then that value. Ming, do you think we can avoid such behavior when MediaExtractor::Create() is called?

(In reply to Bruce Sun [:brsun] from comment #38) Sorry, i have no idea about this.

(In reply to Ming from comment #39) > (In reply to Bruce Sun [:brsun] from comment #38) > Sorry, i have no idea about this. MediaExtractor is under frameworks/base. Would you mind having someone to check the possibility at your side?

(In reply to Bruce Sun [:brsun] from comment #40) > (In reply to Ming from comment #39) > > (In reply to Bruce Sun [:brsun] from comment #38) > > Sorry, i have no idea about this. > > MediaExtractor is under frameworks/base. Would you mind having someone to > check the possibility at your side? Okay.:)

Comment on attachment 8409521 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsfileinputstream.patch Review of attachment 8409521 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsFileStreams.cpp @@ +531,5 @@ > NS_IMETHODIMP > nsFileInputStream::Tell(int64_t *aResult) > { > + nsresult rv = nsFileStreamBase::Tell(aResult); > + if (rv == NS_BASE_STREAM_CLOSED && mBehaviorFlags & REOPEN_ON_REWIND) { instead of getting rv from nsFileStreamBase::Tell, can't you just test !mFD as Seek() does? Consider it and make the change if it works for you. @@ +532,5 @@ > nsFileInputStream::Tell(int64_t *aResult) > { > + nsresult rv = nsFileStreamBase::Tell(aResult); > + if (rv == NS_BASE_STREAM_CLOSED && mBehaviorFlags & REOPEN_ON_REWIND) { > + rv = Seek(NS_SEEK_CUR, 0); please comment that this is being done to reopen the file @@ +542,4 @@ > } > > NS_IMETHODIMP > nsFileInputStream::Available(uint64_t *aResult) same comments as tell

Comment on attachment 8409521 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsfileinputstream.patch Review of attachment 8409521 [details] [diff] [review]: ----------------------------------------------------------------- I think the FileStream code is doing the right thing without this patch. Calling available() should tell you how much is available/throw and not implicitly rewind. Otherwise calling code can just go around and around in circles.

10:34:21 AM) mcmanus: I think you need a domfile expert. to help here.. I'm not sure about the whole story around close_on_eof but it seems that's the crux of the issue.. you don't want it in your usecase but the domfile folks definitely intended that.

Comment on attachment 8408914 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream.patch Set review? on this patch.

I'm very worried about changing the file stream code. The risk of regressions is very large.

Bruce, in comment 27 you see a failure in MediaStreamSource::readAt(). Can you show the stack trace for that failure? It seems to me like the media code is trying to walk past the end of the file incorrectly.

I assume the CLOSE_ON_EOF is there to avoid the file being locked forever on Windows just because some web page read it, but I didn't exactly write that code.... That said, it seems to me that we should fix nsMultiplexInputStream::Seek to work correctly. I assume Seek(0, SEEK_SET) works already when the multiplex stream has been read all the way through, since form submission and HTTP depend on that. But I can't tell how, since I'd think we end up in the i < oldCurrentStream case all the time in the loop in Seek(), which should cause us to call Tell(), which should fail if the file stream is closed...

Comment on attachment 8408912 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_sfilestreamflags.patch Review of attachment 8408912 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should do this.

Comment on attachment 8408914 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream.patch Review of attachment 8408914 [details] [diff] [review]: ----------------------------------------------------------------- This looks generally ok but there are some real problems here. ::: xpcom/io/nsMultiplexInputStream.cpp @@ +184,5 @@ > + if (rv == NS_BASE_STREAM_CLOSED) { > + nsCOMPtr<nsISeekableStream> stream = > + do_QueryInterface(mStreams[i]); > + if (!stream) { > + return NS_ERROR_FAILURE; I'd return rv. @@ +186,5 @@ > + do_QueryInterface(mStreams[i]); > + if (!stream) { > + return NS_ERROR_FAILURE; > + } > + rv = stream->Seek(NS_SEEK_CUR, 0); Now that you've tried seeking you need to also actually do the Available call again... Otherwise you're not setting |streamAvail| at all. This pattern repeats in all the blocks below. Have you tested this? It looks to me like you're always touching uninitialized memory right now... @@ +394,5 @@ > } > else { > rv = stream->Tell(&streamPos); > + if (rv == NS_BASE_STREAM_CLOSED) { > + rv = stream->Seek(NS_SEEK_CUR, 0); Now you need to error check and then call Tell again.

I'm switching this to POVB for partners to solve this on their side since this is one of remaining 1.3T blockers and it seems that we may not be able to have a Gecko solution soon. James, Bruce has provided a workaround to Ming in comment 38 / comment 40. Could you help us to confirm with Ming to see if he needs any further support to fix this issue?

(In reply to Eric Chou [:ericchou] [:echou] from comment #51) > I'm switching this to POVB for partners to solve this on their side since > this is one of remaining 1.3T blockers and it seems that we may not be able > to have a Gecko solution soon. > > James, Bruce has provided a workaround to Ming in comment 38 / comment 40. > Could you help us to confirm with Ming to see if he needs any further > support to fix this issue? To clarify, we definitely want to find the root cause and get this solved in Gecko. Bruce will keep working on this closely with reviewers(thanks to Patrick, BZ, Ben and Paul). The comment I left in comment 51 was just a workaround for our partner to keep moving 1.3T into the next run of testing.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #47) > Bruce, in comment 27 you see a failure in MediaStreamSource::readAt(). Can > you show the stack trace for that failure? It seems to me like the media > code is trying to walk past the end of the file incorrectly. The file size is 16360 bytes in total, but MediaStreamSource::readAt() uses offset=17183 to read. Breakpoint 2, android::MediaStreamSource::readAt (this=0x43842c80, offset=17183, data=0x4479b86c, size=4) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/OmxDecoder.cpp:248 248 return size - todo; (gdb) bt #0 android::MediaStreamSource::readAt (this=0x43842c80, offset=17183, data=0x4479b86c, size=4) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/OmxDecoder.cpp:248 #1 0x4273545a in Resync (source=..., match_header=<value optimized out>, inout_pos=0x4479bcc8, post_id3_pos=<value optimized out>, out_header=0x4479bcd4) at frameworks/base/media/libstagefright/MP3Extractor.cpp:168 #2 0x4273556a in android::SniffMP3 (source=..., mimeType=0x4479bd0c, confidence=0x4479bd08, meta=0x4479bd04) at frameworks/base/media/libstagefright/MP3Extractor.cpp:600 #3 0x42732a7c in android::DataSource::sniff (this=0x43842c80, mimeType=<value optimized out>, confidence=0x4479bd44, meta=0x4479bd4c) at frameworks/base/media/libstagefright/DataSource.cpp:78 #4 0x4274006e in android::MediaExtractor::Create (source=..., mime=0x0) at frameworks/base/media/libstagefright/MediaExtractor.cpp:65 #5 0x416878fa in mozilla::MediaOmxReader::InitOmxDecoder (this=0x40446b50) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/MediaOmxReader.cpp:96 #6 0x41687770 in mozilla::MediaOmxReader::ReadMetadata (this=0x40446b50, aInfo=0x43842c80, aTags=0x4479be24) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/MediaOmxReader.cpp:116 #7 0x41655704 in mozilla::MediaDecoderStateMachine::DecodeMetadata (this=0x43b519f0) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/MediaDecoderStateMachine.cpp:1916 #8 0x416570d0 in mozilla::MediaDecoderStateMachine::DecodeThreadRun (this=0x43b519f0) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/MediaDecoderStateMachine.cpp:531 #9 0x40ef5de8 in nsRunnableMethodImpl<void (nsObserverService::*)(), void, true>::Run (this=<value optimized out>) at ../../dist/include/nsThreadUtils.h:383 #10 0x40f0bf0c in nsThread::ProcessNextEvent (this=0x45c9a7c0, mayWait=<value optimized out>, result=0x4479beaf) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/threads/nsThread.cpp:612 #11 0x40edeca0 in NS_ProcessNextEvent (thread=0x40446b50, mayWait=true) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/glue/nsThreadUtils.cpp:263 #12 0x40f0c432 in nsThread::ThreadFunc (arg=<value optimized out>) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/threads/nsThread.cpp:246 #13 0x40b0b260 in _pt_root (arg=<value optimized out>) at /home/bruce_sun/Source/B2G-tarako/gecko/nsprpub/pr/src/pthreads/ptthread.c:205 #14 0x400fd158 in __thread_entry (func=0x40b0b1c9 <_pt_root>, arg=0x45d73180, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217 #15 0x400fccac in pthread_create (thread_out=<value optimized out>, attr=0x4684bcdc, start_routine=0x40b0b1c9 <_pt_root>, arg=0x45d73180) at bionic/libc/bionic/pthread.c:357 #16 0x00000000 in ?? ()

(In reply to Bruce Sun [:brsun] from comment #53) > (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #47) > > Bruce, in comment 27 you see a failure in MediaStreamSource::readAt(). Can > > you show the stack trace for that failure? It seems to me like the media > > code is trying to walk past the end of the file incorrectly. > > The file size is 16360 bytes in total, but MediaStreamSource::readAt() uses > offset=17183 to read. After that, that next readAt() fails even the offset falls within the correct range (but the size is not good): Breakpoint 1, android::MediaStreamSource::readAt (this=0x43842c80, offset=16339, data=0x4479b883, size=1021) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/OmxDecoder.cpp:244 244 return ERROR_IO; (gdb) bt #0 android::MediaStreamSource::readAt (this=0x43842c80, offset=16339, data=0x4479b883, size=1021) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/OmxDecoder.cpp:244 #1 0x427353d2 in Resync (source=..., match_header=<value optimized out>, inout_pos=0x4479bcc8, post_id3_pos=<value optimized out>, out_header=0x4479bcd4) at frameworks/base/media/libstagefright/MP3Extractor.cpp:126 #2 0x4273556a in android::SniffMP3 (source=..., mimeType=0x4479bd0c, confidence=0x4479bd08, meta=0x4479bd04) at frameworks/base/media/libstagefright/MP3Extractor.cpp:600 #3 0x42732a7c in android::DataSource::sniff (this=0x43842c80, mimeType=<value optimized out>, confidence=0x4479bd44, meta=0x4479bd4c) at frameworks/base/media/libstagefright/DataSource.cpp:78 #4 0x4274006e in android::MediaExtractor::Create (source=..., mime=0x0) at frameworks/base/media/libstagefright/MediaExtractor.cpp:65 #5 0x416878fa in mozilla::MediaOmxReader::InitOmxDecoder (this=0x40446b50) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/MediaOmxReader.cpp:96 #6 0x41687770 in mozilla::MediaOmxReader::ReadMetadata (this=0x40446b50, aInfo=0x43842c80, aTags=0x4479be24) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/omx/MediaOmxReader.cpp:116 #7 0x41655704 in mozilla::MediaDecoderStateMachine::DecodeMetadata (this=0x43b519f0) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/MediaDecoderStateMachine.cpp:1916 #8 0x416570d0 in mozilla::MediaDecoderStateMachine::DecodeThreadRun (this=0x43b519f0) at /home/bruce_sun/Source/B2G-tarako/gecko/content/media/MediaDecoderStateMachine.cpp:531 #9 0x40ef5de8 in nsRunnableMethodImpl<void (nsObserverService::*)(), void, true>::Run (this=<value optimized out>) at ../../dist/include/nsThreadUtils.h:383 #10 0x40f0bf0c in nsThread::ProcessNextEvent (this=0x45c9a7c0, mayWait=<value optimized out>, result=0x4479beaf) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/threads/nsThread.cpp:612 #11 0x40edeca0 in NS_ProcessNextEvent (thread=0x40446b50, mayWait=true) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/glue/nsThreadUtils.cpp:263 #12 0x40f0c432 in nsThread::ThreadFunc (arg=<value optimized out>) at /home/bruce_sun/Source/B2G-tarako/gecko/xpcom/threads/nsThread.cpp:246 #13 0x40b0b260 in _pt_root (arg=<value optimized out>) at /home/bruce_sun/Source/B2G-tarako/gecko/nsprpub/pr/src/pthreads/ptthread.c:205 #14 0x400fd158 in __thread_entry (func=0x40b0b1c9 <_pt_root>, arg=0x45d73180, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217 #15 0x400fccac in pthread_create (thread_out=<value optimized out>, attr=0x4684bcdc, start_routine=0x40b0b1c9 <_pt_root>, arg=0x45d73180) at bionic/libc/bionic/pthread.c:357 #16 0x00000000 in ?? ()

Refine attachment 8408914 [details] [diff] [review] as comment 50.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #50) > Have you tested this? It looks to me like you're always touching > uninitialized memory right now... You are absolutely right, there should be some dangerous parts here...but I've tested this and it mysteriously worked somehow... I've modified these defects in attachment 8412528 [details] [diff] [review].

Comment on attachment 8412528 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_v2.patch Change feedback? flag to review? flag.

Comment on attachment 8412528 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_v2.patch Review of attachment 8412528 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, Boris do you agree? ::: xpcom/io/nsMultiplexInputStream.cpp @@ +76,5 @@ > nsIInputStream, > nsISeekableStream) > > +static nsresult > +AvailableWithSeek(nsIInputStream *stream, nsISeekableStream *seekable, uint64_t *_retval) I don't think there's much reason to have the |AvailableWithSeek| or |TellWithSeek| functions, just inline them into the other two.

Comment on attachment 8412528 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_v2.patch FYI this patch alone (with some tweaks to remove the extra functions) allows the ringtone to be played on my tarako.

Comment on attachment 8412528 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_v2.patch I'm going to punt to someone who might be able to get to this faster than me... In particular, I haven't had a chance to figure out why seeking to 0 works even without this patch, which makes me a bit worried about just stamping it.

(In reply to Eric Chou [:ericchou] [:echou] from comment #51) > I'm switching this to POVB for partners to solve this on their side since > this is one of remaining 1.3T blockers and it seems that we may not be able > to have a Gecko solution soon. > > James, Bruce has provided a workaround to Ming in comment 38 / comment 40. > Could you help us to confirm with Ming to see if he needs any further > support to fix this issue? Do you have any patch need me land on my side? Ming don't know how to do.

(In reply to James Zhang from comment #61) > (In reply to Eric Chou [:ericchou] [:echou] from comment #51) > > I'm switching this to POVB for partners to solve this on their side since > > this is one of remaining 1.3T blockers and it seems that we may not be able > > to have a Gecko solution soon. > > > > James, Bruce has provided a workaround to Ming in comment 38 / comment 40. > > Could you help us to confirm with Ming to see if he needs any further > > support to fix this issue? > > Do you have any patch need me land on my side? > Ming don't know how to do. Hi James, I talked with Ming on phone about this few days ago. It seemed he had some ideas about why MediaExtractor::Create() reads 17183-byte offset on the file with 16360 bytes only. If this abnormal behavior can be avoided, we won't have this issue.

(In reply to Bruce Sun [:brsun] from comment #62) referd to comment 54 , i think maybe there is a bug in 'SniffMP3'. We will check it. But even till now i got no patch for it. It will be appreciate for sharing some resolution on this issue.

add alan wang

(In reply to Ming from comment #63) > (In reply to Bruce Sun [:brsun] from comment #62) > referd to comment 54 , i think maybe there is a bug in 'SniffMP3'. > > We will check it. But even till now i got no patch for it. > It will be appreciate for sharing some resolution on this issue. Probably you could find clues by diff Gonk of other devices. Not sure whether it helps or not, but should be a good start point.

I'll wait Wayne's AOSP patch.

I have just done a commit on sprd side. This issue will not be reproduced once the change be merged in.

(In reply to Boris Zbarsky [:bz] from comment #48) > That said, it seems to me that we should fix nsMultiplexInputStream::Seek to > work correctly. I assume Seek(0, SEEK_SET) works already when the multiplex > stream has been read all the way through, since form submission and HTTP > depend on that. http wraps the nsMultiplexInputStream in a buffered input stream (nsStringStream) which is really handling that Seek(). Is that the reason? https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#354 But I can't tell how, since I'd think we end up in the i < > oldCurrentStream case all the time in the loop in Seek(), which should cause > us to call Tell(), which should fail if the file stream is closed...

Comment on attachment 8412528 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_v2.patch Review of attachment 8412528 [details] [diff] [review]: ----------------------------------------------------------------- lgtm with ben's change

> Is that the reason? I wouldn't think so, when the multiplex data is bigger than the buffer.

(In reply to Ming from comment #67) My commit is not allowed to be merged in. So ignore this comment.

Fabrice, can you help to land this review+ patch?

Hi James, (In reply to James Zhang from comment #72) > Fabrice, can you help to land this review+ patch? Although the patch has already got r+, there are still several small pieces which need Bruce to address. After we have the final patch we'll ask for Fabrice's help to merge it into 1.3T. Thank you.

Modified from attachment 8412528 [details] [diff] [review] by following the comment 58. - Inline AvailableWithSeek into AvailableMaybeSeek. - Inline TellWithSeek into TellMaybeSeek. - Add comments in AvailableMaybeSeek and TellMaybeSeek for this extra seeking behaviour. - Add r=mcmanus. TBPL results are all green: https://tbpl.mozilla.org/?tree=Try&rev=49394f3a6236

Please help to land this issue.

Please land it on v1.3t.

Fabrice, we have kicked off v1.4 dophin/shark project, we need land this patch on v1.4.

(In reply to James Zhang from comment #80) > Fabrice, we have kicked off v1.4 dophin/shark project, we need land this > patch on v1.4. So to get new patches that are not 1.4 blockers to land on 1.4, you need to ask for approval‑mozilla‑b2g30 on the patch.

Comment on attachment 8417178 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_checkin_needed.patch NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): feature failed User impact if declined: Can't play ringtone when amr file format is used Testing completed: Test compelete in v1.3t Risk to taking this patch (and alternatives if risky): low, because it only adds function call to process the file stream correctly String or UUID changes made by this patch: none

Comment on attachment 8417178 [details] [diff] [review] bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_checkin_needed.patch Based on the comment 80 taking it in 1.4 for Dolphin

Verified as fixed for v1.3T branch with latest v1.3T Tarako build: v1.3T Environmental Variables: Device: Tarako v1.3T MOZ RIL BuildID: 20140523014001 Gaia: e76fc9fc64a027d84b2ec311fc624f4c3459dca9 Gecko: 52c1f97caee9 Version: 28.1 Firmware Version: SP6821A-Gonk-4.0-5-12 Ringtone played as expected once set and DuT was called.

(In reply to Preeti Raghunath(:Preeti) from comment #83) > Comment on attachment 8417178 [details] [diff] [review] > bug990957_amr_ringtone_fail_refine_nsmultiplexinputstream_checkin_needed. > patch > > Based on the comment 80 taking it in 1.4 for Dolphin Fabrice, it's approval-mozilla-b2g30+ now. Do we have 1.4 branch merge permission?