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

Status: RESOLVED FIXED

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

Description

[Pooja]

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

Comment 1

[Jason Smith [:jsmith]]

QA Wanted to check 1.3.

Comment 2

[Jason Smith [:jsmith]]

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

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Pooja has a patch for the fix, attach away!

Comment 4

[Pooja]

Attachment: FileMediaResource Tell should return error when file is closed

This makes it works for us. Please suggest if there is a better way.

Comment 5

[Preeti Raghunath(:Preeti)]

Hema

Please review patch for ringtone

Comment 6

[Hema Koka [:hema]]

(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

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 8

[vasanth]

Attachment: FileMediaResource Tell returns mSize as offset (end of stream) in case of error

Thanks! To be consistent, also returning mSize when mSeekable is null. Please review again

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 10

[vasanth]

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[vasanth]

Attachment: FileMediaResource Tell returns mSize as offset (end of stream) in case of error

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 14

[vasanth]

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

Comment 15

[Preeti Raghunath(:Preeti)]

Blocks partner testing and patch provided by partner. Making 1.4+

Comment 16

[Preeti Raghunath(:Preeti)]

Roc,

Can you please confirm if you're landing this patch or not?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 19

[Preeti Raghunath(:Preeti)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/58aa8da5a45d

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58aa8da5a45d

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9f1026a78438

Comment 23

[Yeojin Chung [:YeojinC]]

New test case needs to be added. There is no existing test case.

Comment 24

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case written in moztrap:

https://moztrap.mozilla.org/manage/case/14238/