Closed Bug 1090130 Opened 11 years ago Closed 11 years ago

[b2g] can't transcode video file by MediaRecorder API

Categories

(Core :: Audio/Video: Recording, defect)

Product:
Core
Component:
Audio/Video: Recording
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: alwu, Assigned: alwu)

Details

Attachments

(1 file, 13 obsolete files)

Bug1090130_checkin.patch
15.65 KB, patch
Details | Diff | Splinter Review
STR. 1. goto http://people.mozilla.org/~rlin/MediaRecorder.html 2. play 720WebM 3. start record, found browser crashed.
Assignee: nobody → alwu
Keywords: regressionwindow-wanted
dump the callstack on flame 2.2 (gdb) bt #0 0xb5568d32 in InterleaveAndConvertBuffer<short, short> (aOutput=<optimized out>, aChannels=<optimized out>, aVolume=<optimized out>, aLength=<optimized out>, aSourceChannels=<optimized out>) at ../../../../firefox/dom/media/AudioSegment.cpp:27 #1 mozilla::InterleaveAndConvertBuffer (aSourceChannels=0xafa89438, aSourceFormat=<optimized out>, aLength=aLength@entry=512, aVolume=1, aChannels=2, aOutput=0xae8ab918) at ../../../../firefox/dom/media/AudioSegment.cpp:53 #2 0xb558e790 in mozilla::AudioTrackEncoder::InterleaveTrackData (aChunk=..., aDuration=aDuration@entry=512, aOutputChannels=2, aOutput=aOutput@entry=0xae8ab918) at ../../../../../firefox/dom/media/encoder/TrackEncoder.cpp:158 #3 0xb55d72d6 in android::OMXAudioEncoder::Encode (this=0xb1f99e20, aSegment=..., aInputFlags=0) at ../../../../../firefox/dom/media/omx/OMXCodecWrapper.cpp:721 #4 0xb558f618 in mozilla::OmxAudioTrackEncoder::GetEncodedTrack (this=0xb1504d30, aData=...) at ../../../../../firefox/dom/media/encoder/OmxTrackEncoder.cpp:242 #5 0xb558f81c in WriteEncodedDataToMuxer (aTrackEncoder=0xb1504d30, this=0xb1509140) at ../../../../../firefox/dom/media/encoder/MediaEncoder.cpp:290 #6 mozilla::MediaEncoder::WriteEncodedDataToMuxer (this=0xb1509140, aTrackEncoder=0xb1504d30) at ../../../../../firefox/dom/media/encoder/MediaEncoder.cpp:277 #7 0xb558f9b8 in mozilla::MediaEncoder::GetEncodedData (this=0xb1509140, aOutputBufs=aOutputBufs@entry=0xaed4dc6c, aMIMEType=...) at ../../../../../firefox/dom/media/encoder/MediaEncoder.cpp:231 #8 0xb557ddda in mozilla::dom::MediaRecorder::Session::Extract (this=0xb1509100, aForceFlush=aForceFlush@entry=false) at ../../../../firefox/dom/media/MediaRecorder.cpp:460 #9 0xb557df28 in mozilla::dom::MediaRecorder::Session::ExtractRunnable::Run (this=0xafa894f0) at ../../../../firefox/dom/media/MediaRecorder.cpp:242 #10 0xb4d68930 in nsThread::ProcessNextEvent (this=0xb3d967b0, aMayWait=<optimized out>, aResult=0xaed4dd27) at ../../../../firefox/xpcom/threads/nsThread.cpp:830 #11 0xb4d76592 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at /home/alastor/firefox/xpcom/glue/nsThreadUtils.cpp:265 #12 0xb4eb9e96 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0xb1f329d0, aDelegate=0xb1f1c6a0) at ../../../../firefox/ipc/glue/MessagePump.cpp:339 #13 0xb4eae95c in MessageLoop::RunInternal (this=this@entry=0xb1f1c6a0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233 #14 0xb4eaea10 in RunHandler (this=0xb1f1c6a0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226 #15 MessageLoop::Run (this=this@entry=0xb1f1c6a0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200 #16 0xb4d6b190 in nsThread::ThreadFunc (aArg=0xb3d967b0) at ../../../../firefox/xpcom/threads/nsThread.cpp:350 #17 0xb69a4c02 in _pt_root (arg=0xb34fc900) at ../../../../../../firefox/nsprpub/pr/src/pthreads/ptthread.c:212 #18 0xb6ee222c in __thread_entry (func=0xb69a4b69 <_pt_root>, arg=0xb34fc900, tls=0xaed4ddd0) at bionic/libc/bionic/pthread_create.cpp:105 #19 0xb6ee23c4 in pthread_create (thread_out=0xbeba94a4, attr=<optimized out>, start_routine=0xb69a4b69 <_pt_root>, arg=0x78) at bionic/libc/bionic/pthread_create.cpp:224 #20 0x00000000 in ?? ()
It can't be reproduce in the patch version "80e18ff7c7b2". But here is a new crash in the same situation. (gdb) bt #0 0xb5a4291e in GetEncodedData (this=0xb168c580) at ../../../../firefox/dom/media/MediaRecorder.cpp:414 #1 mozilla::dom::MediaRecorder::Session::PushBlobRunnable::Run (this=0xb0aad980) at ../../../../firefox/dom/media/MediaRecorder.cpp:185 #2 0xb5229058 in nsThread::ProcessNextEvent (this=0xb4402630, aMayWait=<optimized out>, aResult=0xbebb5fcf) at ../../../../firefox/xpcom/threads/nsThread.cpp:830 #3 0xb5236cba in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=true) at /home/alastor/firefox/xpcom/glue/nsThreadUtils.cpp:265 #4 0xb537a936 in mozilla::ipc::MessagePump::Run (this=0xb4401c40, aDelegate=0xbebb60d0) at ../../../../firefox/ipc/glue/MessagePump.cpp:140 #5 0xb536f30c in MessageLoop::RunInternal (this=this@entry=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233 #6 0xb536f3c0 in RunHandler (this=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226 #7 MessageLoop::Run (this=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200 #8 0xb5bfa8ba in nsBaseAppShell::Run (this=0xb2228520) at ../../../firefox/widget/nsBaseAppShell.cpp:164 #9 0xb5f1343a in XRE_RunAppShell () at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:713 #10 0xb536f30c in MessageLoop::RunInternal (this=this@entry=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233 #11 0xb536f3c0 in RunHandler (this=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226 #12 MessageLoop::Run (this=this@entry=0xbebb60d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200 #13 0xb5f13866 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>) at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:550 #14 0x0000910e in content_process_main (argc=7, argv=0xbebb6a54) at ../../../../firefox/ipc/app/../contentproc/plugin-container.cpp:158 #15 0xb6f074a4 in __libc_init (raw_args=0xbebb6a50, onexit=<optimized out>, slingshot=0x912d <main(int, char**)>, structors=<optimized out>) at bionic/libc/bionic/libc_init_dynamic.cpp:112 #16 0x00009014 in _start ()
It's another crash that happened in the same operation step. (gdb) bt #0 mozilla::dom::MediaRecorder::Session::DispatchStartEventRunnable::Run (this=0xb024c0c0) at ../../../../firefox/dom/media/MediaRecorder.cpp:215 #1 0xb5166058 in nsThread::ProcessNextEvent (this=0xb4302630, aMayWait=<optimized out>, aResult=0xbeb0afcf) at ../../../../firefox/xpcom/threads/nsThread.cpp:830 #2 0xb5173cba in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at /home/alastor/firefox/xpcom/glue/nsThreadUtils.cpp:265 #3 0xb52b78e8 in mozilla::ipc::MessagePump::Run (this=0xb4301c40, aDelegate=0xbeb0b0d0) at ../../../../firefox/ipc/glue/MessagePump.cpp:99 #4 0xb52ac30c in MessageLoop::RunInternal (this=this@entry=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233 #5 0xb52ac3c0 in RunHandler (this=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226 #6 MessageLoop::Run (this=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200 #7 0xb5b3795a in nsBaseAppShell::Run (this=0xb2128520) at ../../../firefox/widget/nsBaseAppShell.cpp:164 #8 0xb5e504e2 in XRE_RunAppShell () at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:713 #9 0xb52ac30c in MessageLoop::RunInternal (this=this@entry=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233 #10 0xb52ac3c0 in RunHandler (this=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226 #11 MessageLoop::Run (this=this@entry=0xbeb0b0d0) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200 #12 0xb5e5090e in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>) at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:550 #13 0x0000910e in content_process_main (argc=7, argv=0xbeb0ba54) at ../../../../firefox/ipc/app/../contentproc/plugin-container.cpp:158 #14 0xb6e444a4 in __libc_init (raw_args=0xbeb0ba50, onexit=<optimized out>, slingshot=0x912d <main(int, char**)>, structors=<optimized out>) at bionic/libc/bionic/libc_init_dynamic.cpp:112 #15 0x00009014 in _start ()
The reason of the bug is accessing to the unknown memory address. In the OMXAudioEncoder, it didn't consider the condition when the capacity of the audio chunk is larger than the capacity of the buffer. When it happened, the offset of buffer would be larger than its capacity, so theoretically we should get the negative capacity result from the buffer.AvailableSize() function. Unfortunately, this function only returned the unsigned int, therefore, we continued encoding the data into the buffer. It means that we accessed the memory address which we didn't know about, then resulted in the runtime crash.
Attached patch patch v1 (obsolete) — — Splinter Review
Hi, John, Could you help me review my patch? Thanks a lot :)
Attachment #8518720 - Flags: review?(jolin)
Attached patch Bug1090130_v2.patch (obsolete) — — Splinter Review
Sorry for that I forget to delete the space. This is the revised version, thanks!
Attachment #8518720 - Attachment is obsolete: true
Attachment #8518720 - Flags: review?(jolin)
Attachment #8518734 - Flags: review?(jolin)
Comment on attachment 8518734 [details] [diff] [review] Bug1090130_v2.patch Review of attachment 8518734 [details] [diff] [review]: ----------------------------------------------------------------- r- because calculation of |sampleSize| doesn't seems right and there are still potential buffer overruns. Also, It might be better to extract some code out of Encode() to make it more readable. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +633,5 @@ > return OK; > } > > + size_t getCapableSampleNums(size_t sampleSize) { > + return this->AvailableSize() / sampleSize; Nit: 'this->' seems redundant. @@ +675,5 @@ > } > NS_ENSURE_TRUE(result == OK, NS_ERROR_FAILURE); > > size_t sourceSamplesCopied = 0; // Number of copied samples. > + size_t sampleSize = mChannels * sizeof(AudioDataValue) * mResamplingRatio; Is |sampleSize| bytes per sample? If so, why |mResamplingRatio| needed in the calculation? If not, please add comment to explain what this value is. @@ +680,2 @@ > > + // Copy input PCM data to input buffer until queue is empty. Is moving this line necessary? @@ +694,2 @@ > > + if (chunk.IsNull()) Always brace controlled statements. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures @@ +694,3 @@ > > + if (chunk.IsNull()) > + memset(dst, 0, mResamplingRatio * sourceSamplesToCopy * sizeof(AudioDataValue)); Doesn't this still cause buffer overrun when chunk is larger than dst? @@ +696,5 @@ > + memset(dst, 0, mResamplingRatio * sourceSamplesToCopy * sizeof(AudioDataValue)); > + else { > + // Resample if it has a need > + nsAutoTArray<AudioDataValue, 9600> pcm; > + short* resampleSamplesToCopy; Nit: s/short/int16_t/g @@ +705,5 @@ > + mChannels, > + pcm.Elements()); > + resampleSamplesToCopy = reinterpret_cast<short*>(pcm.Elements()); > + remainingSamplesToCopy = pcm.Length(); > + } Indention: one space missed before '}' @@ +713,5 @@ > + size_t samplesToCopy = (remainingSamplesToCopy > capableSamplesToCopy)? > + capableSamplesToCopy : remainingSamplesToCopy; > + if (samplesToCopy != 0) { > + if (mResampler) > + speex_resampler_process_interleaved_int(mResampler, resampleSamplesToCopy, Won't resampling change the length of input data? I.e., samplesToCopy != dstSamplesCopied and there could still be buffer overrun? I think you should resample the whole chunk first then use the output length to initialize |remainingSamplesToCopy| before this loop.
Attachment #8518734 - Flags: review?(jolin) → review-
About the alert() issue, which will clear the rest tasks in main thread. I think we can new a thread named mainthreadWrapper. Move all runnables which were executed on mainthread to this thread, and at the end of the runnables, do "sync dispatch" to mainthread.
Attached patch Bug1090130_v2.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch, could you help me review it? And it's a short introduce for this revision. Thanks a lot :) =============================================================================== First, it would adjust the |capableSamplesToCopy| depend on its resampling-rate. Then interleave all data and store them into a temporary buffer, because we needed to access the buffer address freely and repeatly in order to save the data from temporary buffer to destination buffer if the chunk size is larger than the capacity of destination buffer. Replace the destination buffer when it was full. we repeated these step until all data was finished. ===============================================================================
Attachment #8518734 - Attachment is obsolete: true
Attachment #8519788 - Flags: review?(jolin)
Comment on attachment 8519788 [details] [diff] [review] Bug1090130_v2.patch Review of attachment 8519788 [details] [diff] [review]: ----------------------------------------------------------------- r- because of some logic errors. Also suggest to break the Encode() function into smaller pieces. For example, check whether (resampled) chunk size is larger than OMX buffer size or not first and then use different copy strategies (functions). ::: dom/media/omx/OMXCodecWrapper.cpp @@ +700,3 @@ > > + if (chunk.IsNull()) { > + memset(dst, 0, mResamplingRatio * capableSamplesToCopy); Buffer overrun seems fixed, but is (mResamplingRatio * capableSamplesToCopy) the correct length? What if there are more samples in chunk than dst can hold? @@ +714,5 @@ > + capableSamplesToCopy : remainingSamplesToCopy; > + if (samplesToCopy != 0) { > + // Resampling if it needs, or push the data into destination buffer. > + if (mResampler) { > + speex_resampler_process_interleaved_int(mResampler, samplesToCopyFromPCM, s/samplesToCopyFromPCM/samplesToCopyFromPCM * mChannels/? @@ +717,5 @@ > + if (mResampler) { > + speex_resampler_process_interleaved_int(mResampler, samplesToCopyFromPCM, > + &samplesToCopy, dst, &dstSamplesCopied); > + // Move the pt to the position where samples are not yet resampled. > + samplesToCopyFromPCM += static_cast<int16_t>(samplesToCopy); Nit: didn't see why casting is necessary. @@ +719,5 @@ > + &samplesToCopy, dst, &dstSamplesCopied); > + // Move the pt to the position where samples are not yet resampled. > + samplesToCopyFromPCM += static_cast<int16_t>(samplesToCopy); > + } else { > + for (int samlpedCopied = 0; samlpedCopied < samplesToCopy; samlpedCopied++) for (...) { ... } @@ +720,5 @@ > + // Move the pt to the position where samples are not yet resampled. > + samplesToCopyFromPCM += static_cast<int16_t>(samplesToCopy); > + } else { > + for (int samlpedCopied = 0; samlpedCopied < samplesToCopy; samlpedCopied++) > + *(dst++) = static_cast<AudioDataValue>(*samplesToCopyFromPCM); /*samplesToCopyFromPCM/*samplesToCopyFromPCM++/? Why not memcpy()?
Attachment #8519788 - Flags: review?(jolin) → review-
Comment on attachment 8519788 [details] [diff] [review] Bug1090130_v2.patch Review of attachment 8519788 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/omx/OMXCodecWrapper.cpp @@ +722,5 @@ > + } else { > + for (int samlpedCopied = 0; samlpedCopied < samplesToCopy; samlpedCopied++) > + *(dst++) = static_cast<AudioDataValue>(*samplesToCopyFromPCM); > + dstSamplesCopied = samplesToCopy * mChannels; > + }: What's the ':' for?
Attached patch Bug1090130_v4.patch (obsolete) — — Splinter Review
Hi, John, This patch splits the Encode() function into smaller part, but it has some difference from the way you suggested. Thanks for your help!
Attachment #8519788 - Attachment is obsolete: true
Attachment #8521137 - Flags: review?(jolin)
Comment on attachment 8521137 [details] [diff] [review] Bug1090130_v4.patch Review of attachment 8521137 [details] [diff] [review]: ----------------------------------------------------------------- r- because of some logic errors. IMHO, rewriting the data copying logic as follows might make more sense: --- BEGIN --- dequeue buffer for (chunk in segment) if (null chunk) fill buffer(s) with silence else if (buffer has enough space for chunk data) if (resample) interleave chunk to pcm resample pcm to dst else interleave to dst else interleave chunk to pcm calculate sub-chunk size for (sub-chunk in pcm) if (resample) resample sub-chunk to dst else copy sub-chunk to dst enqueue/dequeue buffer --- END --- The interleave-resample-copy part could even be a subroutine at the expense of always interleaving to pcm. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +635,5 @@ > > + size_t GetCapableSampleNums(size_t sampleSize, float sampleRate) > + { > + MOZ_ASSERT(sampleSize != 0 || sampleRate < 0); > + return AvailableSize() / (sampleSize * sampleRate); It doesn't seems like a InputBufferHelper method to me. Suggest to do the calculation in Encode() directly. @@ +715,5 @@ > + nsAutoTArray<AudioDataValue, 9600> pcm; > + if (!chunk.IsNull()) { > + pcm.SetLength(sampleSize * sourceSamplesToCopy); > + AudioTrackEncoder::InterleaveTrackData(chunk, sourceSamplesToCopy, > + mChannels, pcm.Elements()); Interleave to |pcm| seems unnecessary when dst has enough space for the data in one chunk. Suggest to check that and interleave directly to dst when it's true. @@ +739,5 @@ > + availableSamplesNum = buffer.GetCapableSampleNums(sampleSize, mResamplingRatio); > + if (result == -EAGAIN) { > + // All input buffers are full. Caller can try again later after > + // consuming some output buffers. > + aSegment.RemoveLeading(sourceSamplesCopied); This does nothing in 1st iteration because sourceSamplesCopied won't be updated until line#753. @@ +742,5 @@ > + // consuming some output buffers. > + aSegment.RemoveLeading(sourceSamplesCopied); > + return NS_OK; > + } > + mTimestamp += sourceSamplesCopied * mSampleDuration; Ditto.
Attachment #8521137 - Flags: review?(jolin) → review-
Attached patch Bug1090130_v5.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch. The strategy of this patch is to divide the problem into two control structures depending on "whether buffer space is enough". And I put the condition of handling the null chunk inside these control structures. Very thanks for your help :)
Attachment #8521137 - Attachment is obsolete: true
Attachment #8522131 - Flags: review?(jolin)
Comment on attachment 8522131 [details] [diff] [review] Bug1090130_v5.patch Review of attachment 8522131 [details] [diff] [review]: ----------------------------------------------------------------- Getting closer. :) Just a thought: will moving the whole buffer splitting/filling/rotating logic to InputBufferHelper makes the code cleaner? E.q., add a SendChunkToInputBuffer() method to it. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +664,5 @@ > + > +void > +OMXAudioEncoder::SpiltIntoSubChunk(size_t bytesToCopy, size_t sampleSize, > + float mResamplingRatio, > + size_t bufferAvailable, suggest to reduce number of parameters: combine sample size and resampling ratio, and pass buffer to get available size & capacity from it. @@ +750,4 @@ > while (!iter.IsEnded()) { > AudioChunk chunk = *iter; > + size_t sourceSamplesToCopy = chunk.GetDuration(); > + size_t bytesToCopy = sampleSize * sourceSamplesToCopy; incomplete calculation: should ' * mResamplingRatio' if resampling is needed. @@ +764,2 @@ > } > + // Update buffer's offset and get next chunk. Nit: s/buffer's/buffer/ @@ +764,5 @@ > } > + // Update buffer's offset and get next chunk. > + sourceSamplesCopied += sourceSamplesToCopy; > + buffer.IncreaseOffset(dstSamplesCopied * sizeof(AudioDataValue)); > + iter.Next(); Always do this after copying: move this after the else {} below? @@ +769,2 @@ > } else { > + // When the data size of the chunk is larger than the buffer's capacity, Nit: s/buffer's/buffer/ @@ +769,3 @@ > } else { > + // When the data size of the chunk is larger than the buffer's capacity, > + // we split it into pieces to fill up with the buffer. fill up buffers. @@ +783,5 @@ > + buffer.AvailableSize(),buffer.GetCapacity(), > + &subChunkSizeArr); > + > + // Store each sub-chunk to the buffer and request a new one. > + for (int idx = 0; idx < subChunkSizeArr.Length(); idx++) { Suggest to extract Arr.Length() into a local variable for optimization. @@ +790,5 @@ > + SendSilenceToEncodedBuffer(dst, sourceSamplesToCopy); > + } else { > + SendSubChunkToEncodedBuffer(pcm.Elements(), samplesToCopy, > + dst, &dstSamplesCopied); > + sourceSamplesCopied += samplesToCopy; Missing '}' before this line. @@ +791,5 @@ > + } else { > + SendSubChunkToEncodedBuffer(pcm.Elements(), samplesToCopy, > + dst, &dstSamplesCopied); > + sourceSamplesCopied += samplesToCopy; > + Missing IncreaseOffset(). ::: dom/media/omx/OMXCodecWrapper.h @@ +241,5 @@ > OMXAudioEncoder() MOZ_DELETE; > OMXAudioEncoder(const OMXAudioEncoder&) MOZ_DELETE; > OMXAudioEncoder& operator=(const OMXAudioEncoder&) MOZ_DELETE; > > + inline void SendSilenceToEncodedBuffer(mozilla::AudioDataValue* dst, EncodedBuffer doesn't sound right to me. InputBuffer perhaps? @@ +255,5 @@ > + uint32_t* dstSamplesCopied); > + void SendSubChunkToEncodedBuffer(mozilla::AudioDataValue* source, > + size_t samplesToCopy, > + mozilla::AudioDataValue* dst, > + uint32_t* dstSamplesCopied); Naming convention: argument names should start with 'a'
Attachment #8522131 - Flags: review?(jolin) → review-
Attached patch Bug1090130_v6.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch. There are some difference between the situation of getting normal chunks and large chunks. In the situation of getting normal size chunks, we needed to update the buffer offset after sending data to the buffer because the buffer was not full. But in the situation of getting large size chunks, we forced to get a new buffer after sending data to the buffer. Since the buffer was new, we didn't need to update its offset. You mentioned these questions in comment#15 (No.4 & 9). Hope my explanation could be helpful to review this patch! Finally, still thanks a lot :)
Attachment #8522131 - Attachment is obsolete: true
Attachment #8523680 - Flags: review?(jolin)
Comment on attachment 8523680 [details] [diff] [review] Bug1090130_v6.patch Review of attachment 8523680 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation. We're almost there. What I actually meant in comment 15 is to move the whole chunk data copying code (line#770~826, and perhaps also 751~757 & 842~852) into InputBufferHelper, but that's just my personal taste. :) InputBufferHelper::ReadChunk(...) { ... } ... while (iter) { sampleCopyed += buffer.ReadChunk(...); iter++; } ::: dom/media/omx/OMXCodecWrapper.cpp @@ +658,5 @@ > + > + void SendSilenceToInputBuffer(AudioDataValue* aDst, size_t aSamplesNum) > + { > + memset(aDst, 0, > + aSamplesNum * sizeof(AudioDataValue) * mEncodedInfo.mResamplingRatio); * mEncodedInfo.mChannels? @@ +779,4 @@ > } > + // Update buffer offset and get next chunk. > + sourceSamplesCopied += sourceSamplesToCopy; > + buffer.IncreaseOffset(dstSamplesCopied * sizeof(AudioDataValue)); |dstSamplesCopied| seems not necessary here. Use |sourceSamplesToCopy| instead? Also |mChannels| seems missing when calculating offset. @@ +801,5 @@ > + if (chunk.IsNull()) { > + buffer.SendSilenceToInputBuffer(dst, sourceSamplesToCopy); > + } else { > + buffer.SendSubChunkToInputBuffer(pcm.Elements(), samplesToCopy, > + dst, &dstSamplesCopied); |dstSamplesCopied| seems unnecessary?
Attachment #8523680 - Flags: review?(jolin) → review-
> @@ +779,4 @@ > > } > > + // Update buffer offset and get next chunk. > > + sourceSamplesCopied += sourceSamplesToCopy; > > + buffer.IncreaseOffset(dstSamplesCopied * sizeof(AudioDataValue)); > > |dstSamplesCopied| seems not necessary here. Use |sourceSamplesToCopy| > instead? > Also |mChannels| seems missing when calculating offset. |dstSamplesCopied| is still useful here because the samples would be changed after resampling. > @@ +801,5 @@ > > + if (chunk.IsNull()) { > > + buffer.SendSilenceToInputBuffer(dst, sourceSamplesToCopy); > > + } else { > > + buffer.SendSubChunkToInputBuffer(pcm.Elements(), samplesToCopy, > > + dst, &dstSamplesCopied); > > |dstSamplesCopied| seems unnecessary? Yes, I would remove it.
Attached patch Bug1090130_v7.patch (obsolete) — — Splinter Review
Hi, John, In this patch, I moved the major algorithm to the InputBufferHelper. Thanks a lots for your help :)
Attachment #8523680 - Attachment is obsolete: true
Attachment #8525086 - Flags: review?(jolin)
(In reply to Alastor Wu [:alwu] from comment #18) > > @@ +779,4 @@ > > > } > > > + // Update buffer offset and get next chunk. > > > + sourceSamplesCopied += sourceSamplesToCopy; > > > + buffer.IncreaseOffset(dstSamplesCopied * sizeof(AudioDataValue)); > > > > |dstSamplesCopied| seems not necessary here. Use |sourceSamplesToCopy| > > instead? > > Also |mChannels| seems missing when calculating offset. > > |dstSamplesCopied| is still useful here because the samples would be changed > after resampling. > You're right. Don't know what I was thinking. :$ And in that case, you'll need to * mChannels because dstSamplesCopied is per channel if I understand speex comment [1] correctly. [1] http://dxr.mozilla.org/mozilla-central/source/media/libspeex_resampler/src/speex_resampler.h#212
(In reply to John Lin [:jolin][:jhlin] (PTO Oct. 9 - Oct. 15) from comment #20) > You're right. Don't know what I was thinking. :$ > And in that case, you'll need to * mChannels because dstSamplesCopied is > per channel if I understand speex comment [1] correctly. > > [1] > http://dxr.mozilla.org/mozilla-central/source/media/libspeex_resampler/src/speex_resampler.h#212 Thanks for remind, I already added this in my latest patch :)
Comment on attachment 8525086 [details] [diff] [review] Bug1090130_v7.patch Review of attachment 8525086 [details] [diff] [review]: ----------------------------------------------------------------- r+ IF the resampled output length problem (lack of # of channels) is addressed. Strongly suggest to add some comments though. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +660,5 @@ > + { > + return mCapicity; > + } > + > + void SendSilenceToInputBuffer(AudioDataValue* aDst, size_t aSamplesNum) Nit: rename to SendSilenceToBuffer()? Nit: aDst seems unnecessary (use GetPointer() instead)? @@ +666,5 @@ > + memset(aDst, 0, aSamplesNum * sizeof(AudioDataValue) > + * mOMXAEncoder.mResamplingRatio * mOMXAEncoder.mChannels); > + } > + > + void SendChunkToInputBuffer(AudioChunk* aChunk, size_t aSourceSamplesToCopy, Could you add some comments for this function, especially for output parameters? Nit: rename to SendChunkToBuffer()? Nit: rename aSourceSamplesCopy to aSamplesNum for consistency with SendSilenceToInputBuffer()? @@ +667,5 @@ > + * mOMXAEncoder.mResamplingRatio * mOMXAEncoder.mChannels); > + } > + > + void SendChunkToInputBuffer(AudioChunk* aChunk, size_t aSourceSamplesToCopy, > + size_t aBytesToCopy, AudioDataValue* aDst, Nit: get rid of aDst? @@ +668,5 @@ > + } > + > + void SendChunkToInputBuffer(AudioChunk* aChunk, size_t aSourceSamplesToCopy, > + size_t aBytesToCopy, AudioDataValue* aDst, > + uint32_t* aDstSamplesCopied) Nit: suggest to move this to return value and make it # of bytes copied. (I.e., = # of sample * # of channels * size of AudioDataValue)? @@ +679,5 @@ > + pcm.Elements()); > + int16_t* tempSource = reinterpret_cast<int16_t*>(pcm.Elements()); > + speex_resampler_process_interleaved_int(mOMXAEncoder.mResampler, tempSource, > + &aSourceSamplesToCopy, aDst, > + aDstSamplesCopied); *aDstSamplesCopied *= mOMXAEncoder.mChannels; @@ +687,5 @@ > + *aDstSamplesCopied = aSourceSamplesToCopy * mOMXAEncoder.mChannels; > + } > + } > + > + void SendSubChunkToInputBuffer(AudioDataValue* aSource, Nit: rename to SendInterleavedSamplesToInputBuffer()? @@ +688,5 @@ > + } > + } > + > + void SendSubChunkToInputBuffer(AudioDataValue* aSource, > + size_t aSamplesToCopy, Nit: rename to aSamplesNum? @@ +689,5 @@ > + } > + > + void SendSubChunkToInputBuffer(AudioDataValue* aSource, > + size_t aSamplesToCopy, > + AudioDataValue* aDst) Nit: get rid of aDst? @@ +694,5 @@ > + { > + if (mOMXAEncoder.mResampler) { > + int16_t* tempSource = reinterpret_cast<int16_t*>(aSource); > + // It was only useful when we need to update the buffer offset > + uint32_t aDstSamplesCopied; Suggest to rename this to not start with 'a'. Also, the comment sounds inaccurate here. (It's just a placeholder.) @@ +705,5 @@ > + memcpy(aDst, aSource, > + aSamplesToCopy * sizeof(AudioDataValue) * mOMXAEncoder.mChannels); > + } > + } > + Add some comment for this function? @@ +721,5 @@ > + bufferAvailable = bufferCapabity; > + } > + } > + > + BufferState ReadChunk(AudioChunk aChunk, size_t* aSourceSamplesCopied) Add some comment for this function, especially for the output parameter. Also suggest to use AudioChunk& for 1st parameter. @@ +728,5 @@ > + size_t sourceSamplesToCopy = chunk.GetDuration(); > + size_t bytesToCopy = sourceSamplesToCopy * mOMXAEncoder.mResamplingRatio > + * mOMXAEncoder.mChannels * sizeof(AudioDataValue); > + > + AudioDataValue* dst = reinterpret_cast<AudioDataValue*>(GetPointer()); Nit: get rid of dst? @@ +771,5 @@ > + // Not enough space left in input buffer. Send it to encoder and get a > + // new one. > + status_t result = Enqueue(mOMXAEncoder.mTimestamp, > + mEInfo.mInputFlags & ~OMXCodecWrapper::BUFFER_EOS); > + if (result != OK) { return BUFFER_FAIL; } Nit: if { ... } @@ +783,5 @@ > + } > + mOMXAEncoder.mTimestamp += samplesToCopy * mOMXAEncoder.mSampleDuration; > + // Update the data address of new buffer > + dst = reinterpret_cast<AudioDataValue*>(GetPointer()); > + if (result != OK) { return BUFFER_FAIL; } Nit: if { ... } @@ +838,5 @@ > AudioSegment::ChunkIterator iter(const_cast<AudioSegment&>(aSegment)); > while (!iter.IsEnded()) { > + BufferState result = buffer.ReadChunk(*iter, &sourceSamplesCopied); > + if (result == WAIT_FOR_NEW_BUFFER) { return NS_OK; } > + else if (result == BUFFER_FAIL) { return NS_ERROR_FAILURE; } Nit: if { ... } else { ... }
Attachment #8525086 - Flags: review?(jolin) → review+
Comment on attachment 8525086 [details] [diff] [review] Bug1090130_v7.patch Review of attachment 8525086 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/omx/OMXCodecWrapper.cpp @@ +777,5 @@ > + result = Dequeue(); > + if (result == -EAGAIN) { > + // All input buffers are full. Caller can try again later after > + // consuming some output buffers. > + mEInfo.mSegment.RemoveLeading(*aSourceSamplesCopied); Could you move this to line#841? That way EncodedInfo.mSegement is not needed and you can get rid of EncodedInfo and just pass input flags to helper class.
Attached patch Bug1090130_v8.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch, thanks a lots :)
Attachment #8525086 - Attachment is obsolete: true
Attachment #8525721 - Flags: review?(jolin)
Comment on attachment 8525721 [details] [diff] [review] Bug1090130_v8.patch Review of attachment 8525721 [details] [diff] [review]: ----------------------------------------------------------------- Many thanks for the hard work! And sorry for not being clear enough when I said 'add more comments'. IMO, Send*() and SpiltIntoSubChunk() don't really need documentation comments since they're short and only used by the helper class itself. Some inline comments in the code would be good enough. Documenting ReadChunk(), on the other hand, is a good idea. Please see comments below for further suggestions. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +651,5 @@ > + /** void SendSilenceToBuffer(aSamplesNum) > + * Send slince auido data when the chunk is null. > + * @param aSamplesNum : the samples number of the audio chunk > + */ > + void SendSilenceToBuffer(size_t aSamplesNum) Suggest to remove documentation comments and make it private. @@ +664,5 @@ > + * @param aSource : audio chunk data > + * @param aSamplesNum : the samples number of the audio chunk > + * @output : samples number of ACTUALLY saved in the buffer > + */ > + uint32_t SendChunkToBuffer(AudioChunk& aSource, size_t aSamplesNum) Ditto. @@ +685,5 @@ > + AudioTrackEncoder::InterleaveTrackData(aSource, aSamplesNum, > + mOMXAEncoder.mChannels, dst); > + } > + dstSamplesCopied *= mOMXAEncoder.mChannels; > + return dstSamplesCopied; Add comment here to describe the return value, such as: "Returns the number of samples written to buffer." Still think returning 'how many bytes written to buffer' looks more clear to me, though. :) @@ +694,5 @@ > + * @param aSource : audio chunk data > + * @param aSamplesNum : the samples number of the audio chunk > + * @output : samples number of ACTUALLY saved in the buffer > + */ > + uint32_t SendInterleavedSubChunkToBuffer(AudioDataValue* aSource, Suggest to remove documentation comments and make it private. BTW, you don't really need the return value, right? @@ +720,5 @@ > + * Calculate the size of subChunk which is available for the buffer. > + * @param aBytesToCopy : audio chunk data > + * @param aSizeArr : the output array of the size of subchunk > + */ > + void SpiltIntoSubChunk(size_t aBytesToCopy, nsAutoTArray<size_t, 10>* aSizeArr) Nit: rename to CalculateSubChunkSizes? @@ +739,5 @@ > + /** output ReadChunk(aChunk, aSourceSamplesCopied) > + * Read chunk data and save it to buffer. > + * @param aChunk : audio chunk data > + * @param aSourceSamplesCopied : the total copied samples number from chunk > + * @output : a state shows whether successfully save data to buffer Existing comments don't actually follow JavaDoc styles. How about: // Read audio data in aChunk, resample them if needed, and then send the result to OMX input buffer (or buffers if one buffer is not enough). // aSourceSampleCopied will be the number of samples that have been read from aChunk. @@ +768,5 @@ > + pcm.SetLength(bytesToCopy); > + AudioTrackEncoder::InterleaveTrackData(aChunk, sourceSamplesToCopy, > + mOMXAEncoder.mChannels, > + pcm.Elements()); > + // Calculate sub-chunk's size. Nit: 'sub-chunk sizes'. Or just rename the function. :) @@ +772,5 @@ > + // Calculate sub-chunk's size. > + nsAutoTArray<size_t, 10> subChunkSizeArr; > + SpiltIntoSubChunk(bytesToCopy, &subChunkSizeArr); > + > + // Store each sub-chunk to the buffer and request a new one. Move this to 778, and 'Fill up buffers with sub-chunk data' sounds more accurate to me. @@ +784,5 @@ > + *aSourceSamplesCopied += samplesToCopy; > + } > + > + // Not enough space left in input buffer. Send it to encoder and get a > + // new one. // Submit the filled-up buffer. @@ +791,5 @@ > + if (result != OK) { > + return BUFFER_FAIL; > + } > + > + result = Dequeue(); A potential problem: this new buffer will have no data if all sub-chunk was already consumed, and be submitted by Enqueue() at line#885.
Attachment #8525721 - Flags: review?(jolin) → review+
Attached patch Bug1090130_v9.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch. The important revision in this patch is to change the order of sending data and requesting buffers. I do the requesting a new buffer first, then sending data to buffer. It could ensures we only enqueue an non-empty buffer. (previous strategy is sending first, then requesting) Very appreciate for review :)
Attachment #8525721 - Attachment is obsolete: true
Attachment #8527450 - Flags: review?(jolin)
Comment on attachment 8527450 [details] [diff] [review] Bug1090130_v9.patch Review of attachment 8527450 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good to me. My only concern is that now the output of CalculateSubChunkSizes() doesn't quite match the logic of the for loop at line#688 and it may seems confusing. Some explanations about why you choose to enqueue non-empty buffer first might be needed. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +33,5 @@ > } while (0) > > namespace android { > > +typedef enum C++ doesn't require typedef in this case. Suggest to use enum BufferState {...} instead. @@ +642,5 @@ > > return OK; > } > > + const size_t GetCapacity() Make this private since it's not used by others. @@ +661,5 @@ > + size_t sourceSamplesToCopy = aChunk.GetDuration(); > + size_t bytesToCopy = sourceSamplesToCopy * mOMXAEncoder.mResamplingRatio > + * mOMXAEncoder.mChannels * sizeof(AudioDataValue); > + size_t bytesCopied = 0; > + if (bytesToCopy < AvailableSize()) { Shouldn't this be '<='?
Attachment #8527450 - Flags: review?(jolin) → review+
Comment on attachment 8527450 [details] [diff] [review] Bug1090130_v9.patch Review of attachment 8527450 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/omx/OMXCodecWrapper.cpp @@ +715,5 @@ > + } > + > + // No audio data left in segment but we still have to feed something to > + // MediaCodec in order to notify EOS. > + void SendSlinceToAvoidNotifyEOS(size_t* aSourceSamplesCopied) This doesn't sound right to me. How about SendEOSToBuffer()?
(In reply to John Lin [:jolin][:jhlin] (PTO Oct. 9 - Oct. 15) from comment #27) > ::: dom/media/omx/OMXCodecWrapper.cpp > @@ +33,5 @@ > > } while (0) > > > > namespace android { > > > > +typedef enum > > C++ doesn't require typedef in this case. Suggest to use enum BufferState > {...} instead. But as I remember, we need to define a enum as type if we want to return or declare it. If I don't add |typedef|, there would get a compile error.
Attached patch Bug1090130_v10.patch (obsolete) — — Splinter Review
Hi, John, Now the logic of handling a large chunk is that check the available chunk size every loop, rather than pre-compute the sub-chunk size. And here is my patch, very appreciate :)
Attachment #8527450 - Attachment is obsolete: true
Attachment #8528095 - Flags: review?(jolin)
Comment on attachment 8528095 [details] [diff] [review] Bug1090130_v10.patch Review of attachment 8528095 [details] [diff] [review]: ----------------------------------------------------------------- Good job! Should be ready for checking-in after the pointer advancing issue is addressed. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +633,5 @@ > } > > + // Read audio data in aChunk, resample them if needed, > + // and then send the result to OMX input buffer (or buffers if one buffer is not enough). > + // aSourceSampleCopied will be the number of samples that have been read from aChunk. Nit: rename to aSamplesRead? @@ +636,5 @@ > + // and then send the result to OMX input buffer (or buffers if one buffer is not enough). > + // aSourceSampleCopied will be the number of samples that have been read from aChunk. > + BufferState ReadChunk(AudioChunk& aChunk, size_t* aSourceSamplesCopied) > + { > + size_t sourceSamplesToCopy = aChunk.GetDuration(); Nit: rename to chunkSamples? @@ +658,5 @@ > + AudioTrackEncoder::InterleaveTrackData(aChunk, sourceSamplesToCopy, > + mOMXAEncoder.mChannels, > + pcm.Elements()); > + // Fill up buffers > + size_t samplesToCopy = 0; Nit: rename to subChunkSamples? @@ +659,5 @@ > + mOMXAEncoder.mChannels, > + pcm.Elements()); > + // Fill up buffers > + size_t samplesToCopy = 0; > + while(GetAvailableSubChunk(bytesToCopy, samplesToCopy)) { Cannot come up with a better name for this, either. :-$ (GetNextSubChunk(), perhaps?) How about adding some comment to explain that what the function does exactly? @@ +660,5 @@ > + pcm.Elements()); > + // Fill up buffers > + size_t samplesToCopy = 0; > + while(GetAvailableSubChunk(bytesToCopy, samplesToCopy)) { > + // Submit the filled-up buffer and request a new buffer. Suggest to move this comment inside if loop. Also, although I understand why you flush before sending, for others it's still not clear enough by merely reading the code. Suggest to add more comment for the while loop. @@ +677,5 @@ > + return BUFFER_FAIL; > + } > + } > + if (aChunk.IsNull()) { > + bytesCopied = SendSilenceToBuffer(sourceSamplesToCopy); s/sourceSamplesToCopy/samplesToCopy/ @@ +679,5 @@ > + } > + if (aChunk.IsNull()) { > + bytesCopied = SendSilenceToBuffer(sourceSamplesToCopy); > + } else { > + bytesCopied = SendInterleavedSubChunkToBuffer(pcm.Elements(), samplesToCopy); See next 2 comments. @@ +776,5 @@ > + speex_resampler_process_interleaved_int(mOMXAEncoder.mResampler, > + tempSource, &aSamplesNum, > + dst, &dstSamplesCopied); > + // Move the pointer to the position where samples are not yet resampled. > + aSource += aSamplesNum * mOMXAEncoder.mChannels; The pointer stored in local variable is moved, but not the original one in ReadChunk(). @@ +780,5 @@ > + aSource += aSamplesNum * mOMXAEncoder.mChannels; > + } else { > + // Directly copy interleaved data into buffer > + memcpy(dst, aSource, > + aSamplesNum * mOMXAEncoder.mChannels * sizeof(AudioDataValue)); Don't you need to move pointer here? @@ +792,5 @@ > + size_t bufferCapabity = GetCapacity(); > + size_t sampleByte = mOMXAEncoder.mChannels * mOMXAEncoder.mResamplingRatio > + * sizeof(AudioDataValue); > + if (aBytesToCopy) { > + if (aBytesToCopy >= bufferCapabity) { Shouldn't it be '>'?
Attachment #8528095 - Flags: review?(jolin) → review-
Attached patch Bug1090130_v11.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch. Very appreciate for review :)
Attachment #8528095 - Attachment is obsolete: true
Attachment #8528967 - Flags: review?(jolin)
Sorry, here is one mistake, the |chunkSamples| should be replaced by |subChunkSamples| in Line#683.
Comment on attachment 8528967 [details] [diff] [review] Bug1090130_v11.patch Review of attachment 8528967 [details] [diff] [review]: ----------------------------------------------------------------- r+ once line#683 and line#689 are fixed. \0/ ::: dom/media/omx/OMXCodecWrapper.cpp @@ +685,5 @@ > + bytesCopied = SendInterleavedSubChunkToBuffer(interleavedSource, subChunkSamples); > + } > + UpdateAfterSendChunk(subChunkSamples, bytesCopied, aSamplesRead); > + // Move to the position where samples are not yet send to the buffer. > + interleavedSource += bytesCopied / sizeof(AudioDataValue); Shouldn't this be '+= subChunkSamples * mOMXAEncoder.mChannels'?
Attachment #8528967 - Flags: review?(jolin) → review+
Comment on attachment 8528967 [details] [diff] [review] Bug1090130_v11.patch Review of attachment 8528967 [details] [diff] [review]: ----------------------------------------------------------------- More review comments. ::: dom/media/omx/OMXCodecWrapper.cpp @@ +685,5 @@ > + bytesCopied = SendInterleavedSubChunkToBuffer(interleavedSource, subChunkSamples); > + } > + UpdateAfterSendChunk(subChunkSamples, bytesCopied, aSamplesRead); > + // Move to the position where samples are not yet send to the buffer. > + interleavedSource += bytesCopied / sizeof(AudioDataValue); Shouldn't this be '+= subChunkSamples * mOMXAEncoder.mChannels'? @@ +788,5 @@ > + return dstSamplesCopied * mOMXAEncoder.mChannels * sizeof(AudioDataValue); > + } > + > + // Calculate the size of subChunk which is available for the buffer. > + bool GetNextSubChunk(size_t& aBytesToCopy, size_t& samplesToCopy) Nit: you may want to mention that aBytesToCopy will also be changed in this function. How about: // Determine the size of sub-chunk (aSamplesToCopy) according to buffer capacity. // For subsequent call, the number of bytes remain to be copied will also be updated in this function. bool NextSubChunk(size_t& aBytesToCopy, size_t& samplesToCopy) @@ +796,5 @@ > + * sizeof(AudioDataValue); > + if (aBytesToCopy) { > + if (aBytesToCopy > bufferCapabity) { > + samplesToCopy = bufferCapabity / sampleByte; > + aBytesToCopy -= bufferCapabity; This looks incorrect when buffer capacity is not multiple of bytes per sample. Should be '-= samplesToCopy * sampleBytes' instead.
Attached patch Bug1090130_v12.patch (obsolete) — — Splinter Review
Hi, John, Here is my revised patch. Thanks a lots for review :)
Attachment #8528967 - Attachment is obsolete: true
Attachment #8529412 - Flags: review?(jolin)
Sorry for a mistake, |samplesToCopy| should be replace by |aSamplesToCopy| in line#793
Comment on attachment 8529412 [details] [diff] [review] Bug1090130_v12.patch Review of attachment 8529412 [details] [diff] [review]: ----------------------------------------------------------------- Great! Please remember to fix line#793. Thanks.
Attachment #8529412 - Flags: review?(jolin) → review+
Attached patch Bug1090130_v13.patch (obsolete) — — Splinter Review
Hi, Randy, Here is my patch, it has been already reviewed by John, Thanks for help :)
Attachment #8529412 - Attachment is obsolete: true
Attachment #8529525 - Flags: review?(rlin)
Comment on attachment 8529525 [details] [diff] [review] Bug1090130_v13.patch John is the author of this part and he reviewed in detail, I think you can set him as reviewer (r=jolin) for this patch. :)
Attachment #8529525 - Flags: review?(rlin) → review+
Attachment #8529525 - Attachment is obsolete: true
Keywords: regressionwindow-wanted → checkin-needed
Hi, could you provide a try link? Thanks!
Flags: needinfo?(alwu)
Keywords: checkin-needed
Here is the try result, thanks! https://tbpl.mozilla.org/?tree=Try&rev=d4dbff5cf259
Flags: needinfo?(alwu)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e3f0394db73
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: