Firefox OS v1.3 Mozilla build ID: 20140312164001 DEFECT DESCRIPTION: ->the MMS can't send. REPRODUCING PROCEDURES: ->Enter"Message"to create a MMS; ->Tap attachment icon to enter"camera",record a max video and add (which size is more than 299.8k); ->Then send the MMS,the MMS can't send.(ko) EXPECTED BEHAVIOUR: ->Should be normally. ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: REPRODUCING RATE: For FT PR, Please list reference mobile's behavior:

FFOS v1.1 can't reproduce this problem. It's a regression. What's more, there are some requirements about the size of video as follows: " MTR-1443 CA-F37 2009 V2 Mandatory MMS Video capture mode: The camera application must support video capture that stops automatically before the file reaches a maximum size of 295KB to match the handset's maximum MMS attachment file size, being configurable as part of the variant process according to TISD MM size."

Just to be sure, what's the bug here? What was the behavior in v1.1? Did the Camera stop recording at 295KB? have you configured a variant, or do you use the 300KB default?

Also leaving qawanted here to see if we can indicate what behavior is seen on 1.3 right now on Buri.

(In reply to Jason Smith [:jsmith] from comment #3) > Also leaving qawanted here to see if we can indicate what behavior is seen > on 1.3 right now on Buri. Following the STR the issue does not occur, the file size caps at around 288-293 KB I am notified about the max file size and the recording stops, I am able to send the MMS without a problem. Environmental Variables: Device: Buri 1.3 MOZ BuildID: 20140325004002 Gaia: b789780c53adaac199787f51615375f721edfef4 Gecko: 37917eb0dfeb Version: 28.0 Firmware Version: V1.2-device.cfg

We need reproducible STR to move this forward.

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #2) > Just to be sure, what's the bug here? What was the behavior in v1.1? Did the > Camera stop recording at 295KB? > > have you configured a variant, or do you use the 300KB default? V1.1 can't reproduce this bug, the Camera will always stop less than 295KB. But if we use v1.3, the camera will stop at more than 295KB, sometimes nearly 299.9KB, then the mms can't send out any more.

So I'd say it's a camera bug. Tian, the issue here is that we don't reproduce the issue on our 1.3 builds, see comment 4, so we need more information from you.

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #7) > So I'd say it's a camera bug. > > Tian, the issue here is that we don't reproduce the issue on our 1.3 builds, > see comment 4, so we need more information from you. Hi Julien - Just discussed with Tian, and she mentioned that if user just copies some large video files(larger than 295KB) to the SD card, and then insert the larger video files into the MMS, the MMS will not be sent out successfully. So even if we fix the camera problem you mentioned in Comment#7, there are still some problems here... Hi Jason - I am verifying this issue now, could you also arrange QA resources and try to reproduce this issue with the following STR? 1. Copy the video(attached on the bugzilla) file into the SD card 2. Create a message, insert that video file, the message will be converted to MMS, the size appears on the screen is 299.7 KB 3. Send this message. 4. Just cannot send out Thanks for your help Vance

Leaving qawanted to try testing this with comment 8's STR.

Vance, I think there are 2 issues here, so we need 2 bugs. Bug 1 (this bug): the Camera is not stopping at 295KB. (which we don't reproduce BTW) Bug 2: we can't send messages with an attachment between 295 and 300KB (is that it?) Please file the second bug and let's focus with Bug 1 here.

Done, submit bug#988768 for "can't send messages with an attachment between 295 and 299KB"

This is the adb log which mms is failed to send out, whose size is more than 299kB.

This is pcap log about mms.

Is this reproducible?

We still need more information from the reporter.

I actually had filed a bug during the bugbash with steps that will reproduce the issue of recording a video exceeding max size with a rate of 100% on a Buri device (which looks to be "Bug 1" from comment 11). I'll paste the steps here. The most important element is step 7, somehow this will allow the camera to record an attachment that is too large to send. I have never seen this issue reproduce when recording normally. --- Repro Steps: 1. Update Buri device to build 20140306134106. 2. Open the Messages app. 3. Tap the pencil and paper icon in the upper right to compose a new message. 4. Tap the paper clip icon in the top right to add an attachment. 5. Select Camera. 6. Tap the Video icon in the bottom right corner to switch to video recording. 7. *IMPORTANT* Lay the device down flat on the table, and leave it there until step 10. 8. Begin recording by tapping the Video Camera icon at the bottom center. 9. Let the video record for at least 2:30 or more. 10. Pick the device up, a message that reads "You have reached the maximum file size for this attachment." will be displayed. 11. Tap "OK" 12. Tap "Select" 13. A message is displayed reading: "The file you have selected is too large" Actual: It is possible to record an attachment video through the messages app that is too large to send via MMS. Expected: It is not possible to record a video exceeding 300kb as an attachment through the messages app, and the "You have reached the maximum file size for this attachment." message is displayed before the file is too large to send. --- The bug for reference: https://bugzilla.mozilla.org/show_bug.cgi?id=980711 --- I tested this on today's 1.3 Buri build using these steps and was able to reproduce this issue, and I initially logged my issue on 1.4 so I'll update the status flags accordingly. Also, attaching logcat while reproducing the issue. 1.3 Environmental Variables: Device: Buri BuildID: 20140326164001 Gaia: 8d159066289fdb4651e13e838fb91fb9226a2cd5 Gecko: 4c7778cc0c98 Version: 28.0 Base Image: V1.2-device.cfg

It looks like the qawanted request for comment 8's STR was filled in bug 988768, so removing qawanted tag for now.

I cannot follow comment 17 - please answer the qawanted request in reference to this bug. Does this bug reproduce or not on 1.3 right now?

I was able to reproduce this issue on todays 1.3 build. The camera continues to record even after the file size exceeds 295 kb. Note: Placing the device flat on the table is a necessary step to reproduce the issue. The video will continue to record until the instant the device is moved. Perhaps video or gyroscope input is necessary to trigger the video size requirement. Environmental Variables: Device: Buri v1.3 Moz RIL BuildID: 20140327004001 Gaia: 8d159066289fdb4651e13e838fb91fb9226a2cd5 Gecko: 4c7778cc0c98 Version: 28.0 Firmware Version: v1.2-device.cfg

So what exactly happens here when the device isn't placed flat on the table?

Laying the device flat doesn't seem to be required to reproduce the issue of the camera recording more than 295 kb as an attachment, just that the camera is covered in some way. In normal use with the camera uncovered, I have never seen this issue reproduce. The camera has always stopped at a point where the video can still be sent over MMS. However, when I covered the camera lens by taping an opaque piece of paper over it (or simply by laying the device flat on the table) so that the camera was only recording solid black, I was able to reproduce this issue.

(In reply to J Zimbrick from comment #22) > > However, when I covered the camera lens by taping an opaque piece of paper > over it (or simply by laying the device flat on the table) so that the > camera was only recording solid black, I was able to reproduce this issue. I know it's April 1, but I'm going to proceed as if it weren't.

I've tested this with a Nexus 4 and the problem occurs there as well: - gecko: b2g-inbound: - gaia: master: I reproduced by taping a dime over the camera lense. Orientation/movement doesn't have any effect, which this is done. If left "recording" for long enough, eventually the blacked-out camera does hit the file-size limit. My _guess_ here is that dark frames get buffered somewhere (codec? recorder?) and then dumped all at once when an interesting frame comes in, or after something times out. Taking to investigate further.

That should be: - gecko: b2g-inbound:275feb8db98a - gaia: camera-new-features:bb7e44e428640f15e1c07b0ab89c712e797fb9ea

The attached log shows recording terminating: 04-01 19:27:40.444 949 3659 I Gecko : recorder-event : info: maximum file size reached 04-01 19:27:40.444 949 957 I Gecko : [../../../../../m-c/b2g-inbound/dom/camera/GonkCameraSource.cpp:629]signalBufferReturned: 0xb481d000 04-01 19:27:40.444 949 957 I Gecko : [../../../../../m-c/b2g-inbound/dom/camera/GonkCameraSource.cpp:606]releaseRecordingFrame 04-01 19:27:40.444 949 3659 I Gecko : recorder-event : track-complete: track 1, 0 (0x00000000) 04-01 19:27:40.444 949 3659 I MPEG4Writer: Received total/0-length (1977/0) buffers and encoded 1975 frames. - video 04-01 19:27:40.454 949 957 I Gecko : [../../../../../m-c/b2g-inbound/dom/camera/GonkCameraSource.cpp:648]read 04-01 19:27:40.454 949 3662 I Gecko : recorder-event : info: maximum file size reached 04-01 19:27:40.454 949 3662 I Gecko : recorder-event : track-complete: track 2, 0 (0x00000000) 04-01 19:27:40.454 949 3662 I MPEG4Writer: Received total/0-length (4559/0) buffers and encoded 4558 frames. - audio

So here's what I'm seeing: - the recording of a ~1m31s video with the camera sensor blacked out is correct--the all-black frames get compressed to almost nothing (looks like 51 bytes/frame); a lot of these can fit into a ~300KiB file. - when the recorder decides to stop, it reports having recorded 135,918 bytes of audio, and 145,792 bytes of video, for a total of 281,710 bytes of total data. - a final fstat() of the file before it closes shows a total size of 322,385 bytes, which is 40,675 more than reported just by the tracks.

So more interesting numbers: - max file size limit is 307,200 bytes - one layer in Track 1 reports having recorded a total of 282,266 bytes - a different layer in Track 1 is estimating a total of 306,226 bytes when the recorder reports MAX_FILESIZE_REACHED - in Track 2, the first layer reports having recorded a total of 282,298 bytes - the other layer in Track 2 estimates a total of 306,258 when reporting MAX_FILESIZE_REACHED - fstat() reports 322,469 bytes So there is significant unaccounted-for overhead. The total estimated size in the iteration before we blow through the size limit is: - Track 1: 306,011 bytes - Track 2: 306,171 bytes This is the condition we're hitting: http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/MPEG4Writer.cpp#1275

Ironically, ICS 4.0.4 has a much simpler implementation of exceedsFileSizeLimit(), which _only_ uses the 95% threshold: http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/MPEG4Writer.cpp#1040 If this were still the case in JB, then (using the numbers in comment 28) 322,496 * 0.95 = 306,346, which is less than the file size limit of 307,200, and so we wouldn't run into this condition. The 'mStreamableFile' member exists in ICS 4.0.4, but the extra condition in exceedsFileSizeLimit() was not introduced until JB 4.2; specifically: https://android.googlesource.com/platform/frameworks/av/+/77e8ae9967a078770416619e99ddb5b010def312

Dears, it's blocking issue to carrier.

Jack, can we have a rationale about this? This happens in a very specific condition that users will likely never see. Nobody will want to record a video showing nothing but a black screen, then send it by MMS.

Dear Julien Wajsberg, For us, this happens in a very commen condition. Only need to record a video in mms until it stop automatically.

Hi Tian/Jack - I think the issue we need to fix is this one Bug#988768, do you agree?

remove from blocker since we should focus on 988768 instead

(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #33) > > I think the issue we need to fix is this one Bug#988768, do you agree? No. The recorder should respect the file size called for in the recording API, and it currently does not.

For my own sanity, I've confirmed that when this issue arises, the file size reported by fstat() matches the blob.size seen in JS. (And both are still too big.) One solution is, bug 988768 notwithstanding (since it has its own issue) is, in Gecko, to simply scale the file size limit by 0.95, thereby effectively restoring the behaviour prior to what it was in the patch mentioned in comment 29.

With this patch (which only affects AOSP 4.2+), the requested file size limit of 307,200 bytes is reduced to 291,840 bytes, and the final file size is 306,070 bytes.

Further to comment 37, however, a non-blacked-out video with the same file size limit and patch applied resulted in a final video only 287,953 bytes (~5.5 seconds) long.

Also, the video and scrubber seem to pause (hesitate?) for a fraction of a second before the end of the video. Not sure what's causing that.

Further to comment 39, I pulled the video file and it plays back fine on my Ubuntu 13.10 X220 laptop, so the glitch is likely something in the player--and is, anyway, a different bug.

Speculation: because the blacked-out video is so much longer than a regular one (~1m30s vs. ~6s), by the time the file size limit is reached, the estimate regarding how much (per-frame, or per-second) metadata needs to be committed to the end of the file is wrong. Deeper digging into the container format is required.

Dear(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #33) > Hi Tian/Jack - > > I think the issue we need to fix is this one Bug#988768, do you agree? It's ok to us, please fix bug#988768. We have solve this problem by limit video recording size but your solution is preferred. Thanks.

I'm not sure at all that bug 988768 would fix this. We could still get a too big image.

Lets get to the bottom of this issue in 1.5

This is a first stab at trying to fix the track estimate calculations to reflect what is actually outputted at the end. A lot was left out of the original algorithm.

Comment on attachment 8414495 [details] [diff] [review] WIP - improve track estimate accuracy, v1 Review of attachment 8414495 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'll test it out today. Just one comment about the indentation in the *_BOX_SIZE calculation. ::: media/libstagefright/MPEG4Writer.cpp @@ +1224,5 @@ > + BASE_STSD_BOX_SIZE + > + BASE_STTS_BOX_SIZE + > + BASE_STSZ_BOX_SIZE + > + BASE_STSC_BOX_SIZE + > + BASE_STCO_BOX_SIZE; I appreciate what you're trying to do here, with the indentation reflecting the structure of the file; but this is likely to look messy to a casual (re)viewer. Is there a better way to document/describe this?

Fixed the style issues noted in Mike's review by just referencing the comment describing the structure at the top. Fixed a few other inconsistent style nits. No functional change.

Comment on attachment 8414524 [details] [diff] [review] WIP - improve track estimate accuracy, v2 [r=mikeh] Review of attachment 8414524 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I'll try to get it tested today. We really need to get you some hardware....

A version of aosmond's patch with extra logcat prints in it.

Andrew, it looks like there's still something in your model that isn't being accounted for: 05-06 14:01:49.481 3256 3403 E MPEG4Writer: mikeh: mMaxFileSizeLimitBytes=302080 05-06 14:01:49.481 3256 3403 E MPEG4Writer: mikeh: mEstimatedMoovBoxSize=3072 05-06 14:01:49.481 3256 3403 E MPEG4Writer: mikeh: track=0xb2cec5c0 : estimateTrackSizeBytes=154115 05-06 14:01:49.481 3256 3403 E MPEG4Writer: mikeh: track=0xb2cecb70 : estimateTrackSizeBytes=143917 05-06 14:01:49.481 3256 3403 E MPEG4Writer: mikeh: nTotalBytesEstimate+1024=302128 ... 05-06 14:01:49.641 3256 3299 E MPEG4Writer: mikeh: final fstat().st_size=315598 The difference is 13,470 bytes.

Fixed some bugs in the calculations, removed additional overhead guessimates as they are no longer necessary and now always calculates the estimate properly whether or not the file is streamable. Accuracy is now within 100 bytes above actual final file size.

Comment on attachment 8424810 [details] [diff] [review] improve track estimate accuracy, v3 Review of attachment 8424810 [details] [diff] [review]: ----------------------------------------------------------------- A few nits and an overall style question. Otherwise this looks good to me. How well does it work? ::: media/libstagefright/MPEG4Writer.cpp @@ +1303,4 @@ > } > > + // if the moov box exceeds the reserved space, we need to append it to the end and the > + // reserved space will be considered wasted nit: capitalize 'if' and put a period at the end of this comment. Also make sure it's wrapped to <=80 columns (I can't tell from here). @@ +1430,5 @@ > + * stsz > + * stsc > + * stco > + */ > + static const int64_t BASE_URL_BOX_SIZE = kBaseMpegBoxSize + 4; Hmm, I see that both THIS_STYLE and kThisStyle are used for constants in this file. Any thoughts/preferences on making all of these use the latter instead? (Sorry to bring it up.) @@ +1480,5 @@ > + BASE_STSC_BOX_SIZE + > + BASE_STCO_BOX_SIZE; > + > + // We add 1 to all of the counts because if we accept the sample, it may increase the size of > + // the file beyond what was requested due to a longer trak box nit: period at the end, make sure this wraps to <=80 cols. @@ +1486,5 @@ > + (mSttsTableEntries->count() + 1) * 8; // stts box size > + mEstimatedTrackSizeBytes += mOwner->use32BitFileOffset() > + ? (mStcoTableEntries->count() + 1) * 4 > + : (mCo64TableEntries->count() + 1) * 8; // stco box size > + mEstimatedTrackSizeBytes += mSamplesHaveSameSize? 4: ((mStszTableEntries->count() + 1) * 4); // stsz box size Can we make all of these magic values named constants as well? @@ +1522,5 @@ > + if (!mIsAudio) { > + mEstimatedTrackSizeBytes += BASE_CTTS_BOX_SIZE + > + (mCttsTableEntries->count() + 1) * 8 + > + BASE_STSS_BOX_SIZE + > + (mStssTableEntries->count() + 1) * 4; Same here for 8 and 4 -- give them names.

BTW, when you upload the revised patch, please tag all of the other attachments as obselete.

Updated based on nits and recent flame testing.

Fix diff format.

Add in missing CHECK nit.

Comment on attachment 8426759 [details] [diff] [review] improve track estimate accuracy, v4.1 Review of attachment 8426759 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the nits below addressed. ::: media/libstagefright/MPEG4Writer.cpp @@ +1448,5 @@ > + static const int64_t kBaseMdhdBoxSize = kBaseMpegBoxSize + 24; > + static const int64_t kBaseStblBoxSize = kBaseMpegBoxSize; > + static const int64_t kBaseMinfBoxSize = kBaseMpegBoxSize; > + static const int64_t kBaseMdiaBoxSize = kBaseMpegBoxSize; > + static const int64_t kBaseTkhdBoxSize = kBaseMpegBoxSize + 96; // 92?? You're going to either need to explain this comment, or remove it. :) @@ +1492,5 @@ > + // trak box. > + mEstimatedTrackSizeBytes += (mStscTableEntries->count() + 1) * > + kEntryStscBoxSize + > + (mSttsTableEntries->count() + 1) * > + kEntrySttsBoxSize; Is the 1-space indentation style used anywhere else in this file? It looks weird. I would structure this as: mEstimatedTrackSizeBytes += (mStscTableEntries->count() + 1) * kEntryStscBoxSize + (mSttsTableEntries->count() + 1) * kEntrySttsBoxSize; Same for the blocks below. @@ +1542,5 @@ > + (mCttsTableEntries->count() + 1) * > + kEntryCttsBoxSize + > + kBaseCttsBoxSize + > + (mStssTableEntries->count() + 1) * > + kEntryStssBoxSize; Similar style here too.

Fix all latest commented nits.

Andrew, please work with mike to get it landed if this is ready.

Could a sherrif take a look at this pull request: https://github.com/mozilla-b2g/platform_frameworks_av/pull/9 I've only created one for master, and can easily create ones for the 1.4, 4.3 and 4.4.2 branches (the changes apply cleanly), but I'm not sure if there is a preferred process for me to follow :). (Note I realize after this I need to convince the vendors to pull from our tree for it to apply to B2G.)

Master: https://github.com/mozilla-b2g/platform_frameworks_av/commit/e4fc42bf2b688baa03cd02dff39798296e11477d

Two more pull requests from 4.3 and 4.4.2 (after discussion on IRC indicating we need them here): https://github.com/mozilla-b2g/platform_frameworks_av/pull/10 https://github.com/mozilla-b2g/platform_frameworks_av/pull/11

b2g-4.3_r2.1: https://github.com/mozilla-b2g/platform_frameworks_av/commit/f9000e9ec6fb84a24ffe049e6a6372c0305ee0f1 b2g-4.4.2_r1: https://github.com/mozilla-b2g/platform_frameworks_av/commit/9c6cb3231dd096df10a11b4d76be3727bdeec08d

Put for up review for integration into AOSP: https://android-review.googlesource.com/#/c/96440/