Closed Bug 1010685 Opened 6 years ago Closed 6 years ago

Could not set smaller AMR clip (<6KB) as ringtone

Categories

(Core :: Audio/Video, defect, P1, critical)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: poojas, Assigned: poojas)

References

()

Details

(Whiteboard: [caf priority: p2][CR 650906])

Attachments

(1 file, 2 obsolete files)

Steps to reproduce
------------------

1.Go to Music app. Play a very small AMR clip.
2.Set it as ringtone.

Actual behavior:
Cannot set it as ringtone. It fails with extractor sniffing and gecko errors

D/ExtendedUtils(  202): Try creating ExtendedExtractor
E/ExtendedExtractor(  202): Failed to instantiate extractor 
D/ExtendedUtils(  202): Couldn't create the extended extractor, return default one
E/GeckoConsole(  202): [JavaScript Warning: "Media resource blob:d36d10ee-ae32-4846-9c9c-d8c31962e986 could not be decoded." {file: "app://system.gaiamobile.org/index.html" line: 0}]

Expected behavior:
The clip should get set as ringtone.


Analysis
nsFileInputStream::Read() closes the file on EOF [1]
To reopen the file, Seek should happen before any further read [2]
MediaStreamSource does seek only when offset is not equal to mResource->Tell() [3]

But When file is closed, mResource->Tell() returns 0, since FileMediaResource::Tell() offset is initialized to 0 [4] and nsInputStreamBase::Tell() can't get position of closed file [5]
So mResource->Tell() becomes equal to offset (both are 0) at [3], seek won''t happen and further read fails.

Initializing FileMediaResource::Tell() offset to -ve value here [4] makes it work.

Sniffing involves reading same file multiple times with different header lengths. Since this media file is very short, (<6KB) EOF can be reached during sniffing sometimes and we hit this corner case while setting ringtone.

[1] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/netwerk/base/src/nsFileStreams.cpp?h=mozilla/v1.4#n477
[2] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/netwerk/base/src/nsFileStreams.cpp?h=mozilla/v1.4#n504
[3] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/content/media/omx/OmxDecoder.cpp?h=mozilla/v1.4#n256
[4] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/content/media/MediaResource.cpp?h=mozilla/v1.4#n1538
[5] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/netwerk/base/src/nsFileStreams.cpp?h=mozilla/v1.4#n72
QA Wanted to check 1.3.
Keywords: qawanted
(In reply to Jason Smith [:jsmith] from comment #1)
> QA Wanted to check 1.3.

Actually we're first going to need a test file to demonstrate the bug here.
Keywords: qawanted
Pooja has a patch for the fix, attach away!
This makes it works for us. Please suggest if there is a better way.
Hema

Please review patch for ringtone
Flags: needinfo?(hkoka)
(In reply to Pooja from comment #4)
> Created attachment 8422934 [details] [diff] [review]
> FileMediaResource Tell should return error when file is closed
> 
> This makes it works for us. Please suggest if there is a better way.

This review probably needs to go to Roc/Ehsan/CJ's. NI'ing them to route to the right reviewer.

Thanks
Hema
Flags: needinfo?(roc)
Flags: needinfo?(ehsan)
Component: Gaia::Ringtones → Video/Audio
Flags: needinfo?(hkoka)
Product: Firefox OS → Core
Comment on attachment 8422934 [details] [diff] [review]
FileMediaResource Tell should return error when file is closed

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

ChannelMediaResource::Tell() returns the last offset (end of stream) when the stream has ended. So I think FileMediaResource should return mSize if mSeekable::Tell fails. Please check the nsresult returned by Tell rather than just setting offset before the Tell call. Thanks!!!
Attachment #8422934 - Flags: review-
Thanks! To be consistent, also returning mSize when mSeekable is null. Please review again
Attachment #8423801 - Flags: review?(roc)
Attachment #8422934 - Attachment is obsolete: true
Comment on attachment 8423801 [details] [diff] [review]
FileMediaResource Tell returns mSize as offset (end of stream) in case of error

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

::: content/media/MediaResource.cpp
@@ +1540,1 @@
>    EnsureSizeInitialized();

Call EnsureSizeInitialized before we return mSize!
Attachment #8423801 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> >    EnsureSizeInitialized();
> 
> Call EnsureSizeInitialized before we return mSize!

You mean to call EnsureSizeInitialized even when mSeekable is null?
EnsureSizeInitialized() uses mInput without null check. When mSeekable is null, there is a chance that mInput might also be null? 

I see mSize is initialized -1 and why returning -1 is a problem when mSeekable is null?
Flags: needinfo?(roc)
mSeekable can't really be null. If it is, (In reply to vasanth from comment #10)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > >    EnsureSizeInitialized();
> > 
> > Call EnsureSizeInitialized before we return mSize!
> 
> You mean to call EnsureSizeInitialized even when mSeekable is null?

Yes.

> EnsureSizeInitialized() uses mInput without null check. When mSeekable is
> null, there is a chance that mInput might also be null?

No. mInput can't be null.

mSeekable can't really be null either --- Open() would have returned an error in that case. But if we're going to return mSize for null mSeekable, we may as well return the real size.
Flags: needinfo?(roc)
Attachment #8423801 - Attachment is obsolete: true
Comment on attachment 8424704 [details] [diff] [review]
FileMediaResource Tell returns mSize as offset (end of stream) in case of error

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

Thanks!!!
Attachment #8424704 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Thanks!!!

Robert, Please give a Try run if needed else we can add "checkin-needed" directly?
Blocks partner testing and patch provided by partner. Making 1.4+
blocking-b2g: 1.4? → 1.4+
Roc,

Can you please confirm if you're landing this patch or not?
Flags: needinfo?(roc)
Comment on attachment 8424704 [details] [diff] [review]
FileMediaResource Tell returns mSize as offset (end of stream) in case of error

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

This is a very safe patch. I think we can land it without a try run.
Attachment #8424704 - Flags: checkin?
Comment on attachment 8424704 [details] [diff] [review]
FileMediaResource Tell returns mSize as offset (end of stream) in case of error

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

OK, I just read the new tree rules, so I guess I'll land this myself.
Attachment #8424704 - Flags: checkin?
Flags: needinfo?(ehsan)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 8424704 [details] [diff] [review]
> FileMediaResource Tell returns mSize as offset (end of stream) in case of
> error
> 
> Review of attachment 8424704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, I just read the new tree rules, so I guess I'll land this myself.

Roc,

Can this land this week? This is blocking partner testing. Thanks.
https://hg.mozilla.org/mozilla-central/rev/58aa8da5a45d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Whiteboard: [CR 650906] → [caf priority: p2][CR 650906]
Flags: in-moztrap?(ychung)
New test case needs to be added. There is no existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Test case written in moztrap:

https://moztrap.mozilla.org/manage/case/14238/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.