1037315 - [MMS] [Music] Audio .ogg and .3gpp mimetypes are shown as video files in SMS app

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[sasikala]

1. Title: Audio .ogg and .3gpp mimetypes are shown as video files in SMS app
2. Precondition: Store audio files with .ogg and .3gpp mimetypes in SDCard
3. Tester's Action: 1.Open SMS application
                    2.Select Attachment button
                    3.Select Music app
                    4.Select any .3gpp or .ogg file
                    5. Check the audio file attached
4. Actual Symptom (ENG.) : The audio file attached is recognized as video
5. Expected: The audio file attached should be recognized and shown as audio in the MMS

Comment 1

[sasikala]

Attachment: Bug_1037315.patch

Hi Dominic,

Kindly provide feedback on attached patch for the issue
1. If the audio file received is shown as video/ogg -> creating a blob and converting it to audio/ogg in music app -> so the attached is received from music will be shown correctly in SMS application
2. If the audio file received is shown as video/3gpp-> creating a blob and converting it to audio/3gpp, so the SMS app receives the correct format
3. This issue happens when sharing while playing and sharing directly from music. So the changes are done in two places to fix the issue.

Thanks!
Sasikala

Comment 2

[Julien Wajsberg [:julienw]]

Shouldn't we change the mime types in the system instead?

On my Linux, I have this:

application/ogg					ogx
audio/ogg					oga ogg opus spx
video/ogg					ogv
audio/3gpp					<nothing>
video/3gpp					3gp

So while the video/ogg should not happen, the video/3gpp is expected.

That said, the "Music" app should be able to open only audio files, right?

Comment 3

[sasikala]

Julien,Can we request needinfo to someone from Gecko team to reply for comment 2.
https://bugzilla.mozilla.org/show_bug.cgi?id=1037315#c2

Comment 4

[Julien Wajsberg [:julienw]]

I think it's more from the Gonk team but I don't know much who are these guys.

Fabrice, can you help?

Comment 5

[[:fabrice] Fabrice Desré]

.3gp and .ogg files can be either music or video files according to https://mxr.mozilla.org/mozilla-central/source/toolkit/content/devicestorage.properties?force=1
I need to dig a bit more into the mime type mapping, but are you sure that the files from comment #0 were actually of the wrong mime type?

Comment 6

[Dominic Kuo [:dkuo]]

Comment on attachment 8454262 [details] [diff] [review]
Bug_1037315.patch

Apologize for the late reply, I was in the media apps work week and didn't have time to response this.

Fabrice is correct in comment 5, .3gp and .ogg files could be either music or video files, this is not just in gecko but also the standard, so we better not to assume some .3gp and .ogg files is video or audio.

Could we do this checking in sms app? this patch only fix the cases in our music app, but if there is a 3rd party also has the capability to provide .3gp and .ogg files, we couldn't force it to follow this trick to make sms display right. And since the sms app already knew what media(video/audio) type it's attaching, it should be able to recognize the type before finishing the pick activity, so we don't have to depend on the returned type from the picked file.

Make sense?

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Dominic Kuo [:dkuo] (Media Apps Work Week, July 14-18) from comment #6)

> Could we do this checking in sms app? this patch only fix the cases in our
> music app, but if there is a 3rd party also has the capability to provide
> .3gp and .ogg files, we couldn't force it to follow this trick to make sms
> display right. And since the sms app already knew what media(video/audio)
> type it's attaching, it should be able to recognize the type before
> finishing the pick activity, so we don't have to depend on the returned type
> from the picked file.
> 
> Make sense?

I don't really follow you; in the SMS app we use the mime type to display as video or audio. As long as the 3rd party app gives us the right mime type, we're fine.

There are 2 cases to attach an attachment:

case 1: the user is in an app, that app uses the "share" activity. We simply get some blobs with their mime type.

case 2: the user is in the SMS app, we request an attachment using the "pick" activity, and this data: `type: ['image/*', 'audio/*', 'video/*']`. So we don't know which type the user requests and only distinguish using the blob we receive.


Is there something I miss, Dominic?

Comment 8

[Dominic Kuo [:dkuo]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> (In reply to Dominic Kuo [:dkuo] (Media Apps Work Week, July 14-18) from
> comment #6)
> 
> > Could we do this checking in sms app? this patch only fix the cases in our
> > music app, but if there is a 3rd party also has the capability to provide
> > .3gp and .ogg files, we couldn't force it to follow this trick to make sms
> > display right. And since the sms app already knew what media(video/audio)
> > type it's attaching, it should be able to recognize the type before
> > finishing the pick activity, so we don't have to depend on the returned type
> > from the picked file.
> > 
> > Make sense?
> 
> I don't really follow you; in the SMS app we use the mime type to display as
> video or audio. As long as the 3rd party app gives us the right mime type,
> we're fine.
>
> There are 2 cases to attach an attachment:
> 
> case 1: the user is in an app, that app uses the "share" activity. We simply
> get some blobs with their mime type.

If the 3rd party app use the mimetype from the deviceStorage api, then I assuming it use file.type, which gecko returns video/ogg for .ogg and video/3gpp for .3gp files. In this case, sms app still display incorrect for video and audio I guess. But I don't think gecko needs to change the returned file.type for both .ogg and .3gp to fix the issue.

> case 2: the user is in the SMS app, we request an attachment using the
> "pick" activity, and this data: `type: ['image/*', 'audio/*', 'video/*']`.
> So we don't know which type the user requests and only distinguish using the
> blob we receive.

I am wrong for this case cause I didn't aware sms is able to attach all the media files, sorry about that.

If this is for better ux in gaia then I am fine with the changes in attachment 8454262 [details] [diff] [review].

Comment 9

[sasikala]

Attachment: index.html

Hi Dominic,

As the patch is already reviewed and OK to take it https://bugzilla.mozilla.org/show_bug.cgi?id=1037315#c8. I have raised a PR.
Kindly review the PR and merge.

This will have good user experience. If user is attaching a audio file but if it is shown as video in SMS this is very confusing to the user.

Comment 10

[sasikala]

Pull request for the patch https://github.com/mozilla-b2g/gaia/pull/23198

Comment 11

[sasikala]

Attachment: index.html

Kindly review the latest patch.

Comment 12

[Dominic Kuo [:dkuo]]

Comment on attachment 8477365 [details]
index.html

sasikala, the patch looks okay but with some noticeable issues, like wrong indent and no comments, would you please address them and set review to me again? thanks.

Comment 13

[sasikala]

Hello Dominic,
Thank you for review comments. I will address these upload again.
Thanks!

Comment 14

[sasikala]

Comment on attachment 8477365 [details]
index.html

Hi Dominic,

Updated the review comments.
1. Added comment for understanding
2. Modified the operator (===)
3. Verified the Indentation

Kindly review.

Thanks!

Comment 15

[Dominic Kuo [:dkuo]]

sasikala, I have commented my idea on https://github.com/mozilla-b2g/gaia/pull/23198#issuecomment-54282746, would you please read it and see if the idea make sense to you then make the changes? thanks!

Comment 16

[sasikala]

Hello Dominic,
The idea which you have shared is good.
Thanks!

Comment 17

[Dominic Kuo [:dkuo]]

sasikala, if my idea looks good to you, please address it and I will review it again then help to land it, thanks!

Comment 18

[sasikala]

Comment on attachment 8477365 [details]
index.html

Hi Dominic,

Updated the patch as per your idea.
Kindly review the changes and land it.

Thanks!

Comment 19

[Dominic Kuo [:dkuo]]

Comment on attachment 8477365 [details]
index.html

sasikala, thanks for addressing those issues and now the code looks good to me!(only a minor nit in the comment). Also, because the mimetype |audio/3gpp| was not supported by the music app's open activity, so we should add it to the manifest.webapp: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/manifest.webapp#L30 or there is no activity to support opening the file/blob that has |audio/3gpp| mimetype.

Please needinfo me after you address the new issues and I will land it for you, thanks!(The commit message seems not following the gaia policy and please also fix it, such as: Bug XXXXXXX - [Music] "How you fix this bug")

Comment 20

[sasikala]

Hi Dominic,

I have one doubt regarding the mimetype |audio/3gpp| support in music application.
Does the |audio/3gpp| mimetype is supported by the platform because even after adding the mimetype in music manifest -> it's giving "No application available to open this filetype" error.

If the platform is not supported -> i think it's better to remove the conversion for video/3gpp to audio/3gpp. Please provide your opinion on this.

Thanks!

Comment 21

[Dominic Kuo [:dkuo]]

(In reply to sasikala from comment #20)
> Hi Dominic,
> 
> I have one doubt regarding the mimetype |audio/3gpp| support in music
> application.
> Does the |audio/3gpp| mimetype is supported by the platform because even
> after adding the mimetype in music manifest -> it's giving "No application
> available to open this filetype" error.
> 
> If the platform is not supported -> i think it's better to remove the
> conversion for video/3gpp to audio/3gpp. Please provide your opinion on this.
> 
> Thanks!

I guess you didn't |make reset-gaia| so you are unable to apply the changes in music app's manifest.webapp, it requires the procedures for re-installing so |make install-gaia| does not really completely re-install it, try |make reset-gaia| to flash whole gaia and you should be able to open the file that has |audio/3gpp| mimetype, with the change I mentioned in comment 19. Let me know if you still have any problem, thanks.

Comment 22

[Julien Wajsberg [:julienw]]

As a workaround you can also use my script in [1] and run "addpref rehash-manifest"; this will change a pref that triggers rehashing all manifests without needing to reset the phone.

[1] https://github.com/julienw/config-files/blob/master/addpref

Comment 23

[Dominic Kuo [:dkuo]]

Sasikala, please address the issues I mentioned in comment 19 then let me know after your patch is updated, and thanks to Julien that his script should help you to test your patch.

Comment 24

[sasikala]

Hi Dominic, Whenever we are trying to play the content which has audio/3gpp mimetype the music application is launching but it's entering into error case in playBlob function.

function playBlob(blob) {
  PlayerView.onerror = function invalid() {
    alert(navigator.mozL10n.get('audioinvalid'));
    done();
  };
}
Because of this it is showing the error message.

Comment 25

[Dominic Kuo [:dkuo]]

(In reply to sasikala from comment #24)
> Hi Dominic, Whenever we are trying to play the content which has audio/3gpp
> mimetype the music application is launching but it's entering into error
> case in playBlob function.
> 
> function playBlob(blob) {
>   PlayerView.onerror = function invalid() {
>     alert(navigator.mozL10n.get('audioinvalid'));
>     done();
>   };
> }
> Because of this it is showing the error message.

If you have successfully triggered the music open activity, the changes in the manifest.webapp does work, and the error your saw is probably the <audio> element refuse to decode some blob that is wrapped like this, which I am not sure either.

Would you please attach your test file here, or send to my email directly? let's see if the test file cause that issue or it's an audio element error, thanks!

Comment 26

[sasikala]

Attachment: Voice00004(1).3gp

Hi Dominic, 

Attached the .3gp file which is used.

Thanks!

Comment 27

[Dominic Kuo [:dkuo]]

(In reply to sasikala from comment #26)
> Created attachment 8492093 [details]
> Voice00004(1).3gp
> 
> Hi Dominic, 
> 
> Attached the .3gp file which is used.
> 
> Thanks!

With the patch(attachment 8477365 [details]) and adding |audio.3gpp| to the manifest.webapp, I am able to play the test file(attachment 8492093 [details]) in sms attachment with music's open activity without any error, also when the test file appears in the regular music app, it can be played normally as well.

Sasikala, what device and branch you are using? I guess it might be different gecko caused your problem because I couldn't reproduce it on master.

Comment 28

[sasikala]

Hi Dominic, We are using Gecko-32 version and Gaia-2.0. 
May i know which Gecko version you are using?

Comment 29

[sasikala]

We are using Gecko-32 version and Gaia-2.0. As per your comment you are checking on master.
Will you please provide the status for v2.0?

Comment 30

[Dominic Kuo [:dkuo]]

I am using master for both gecko(35) and gaia(2.2) on my flame.

Comment 31

[sasikala]

Comment on attachment 8477365 [details]
index.html

Hi Dominic,
Thank you for information.
I have updated the PR as per your comments(https://github.com/SasikalaParuchuri/gaia/commit/076f0bb1eef988cb4ee8f361098bd431e9374ca4). Please review and land it on master.
Thanks!

Comment 32

[Dominic Kuo [:dkuo]]

Comment on attachment 8477365 [details]
index.html

Sasikala, thanks for addressing those issues, the code looks good to me now, would you please rebase your PR cause it seems conflict now? and after the gaia try passed, I will merge it for you.

Comment 33

[sasikala]

Dominic, I have done code rebase on same branch and there are no conflicts while rebasing.
Please check same pull request once and let me know if anything missing

Comment 34

[Dominic Kuo [:dkuo]]

Sasikala, sorry to info you but I still cannot merge your PR because the merge button is gray-out, which means the PR has conflicts, would you please rebase to the latest master then force push to your branch again? thanks!

Comment 35

[sasikala]

Hi Dominic,
Will you please let me know the commands for rebasing the code?

I have done the below commands:
1) git fetch upstream 
2) git rebase upstream/master
3) Fix the conflicts if any. Not found any conflicts
4) git push -f origin YOUR_BRANCH_NAME

Let me know the commands. I will try rebasing once again if same issue happens i will raise a new PR and upload it.

Thanks!

Comment 36

[Julien Wajsberg [:julienw]]

I'm doing this the same way.

Please add checkin-needed once you did this, I think the sheriff will take the time to rebase manually if needed and if there is no conflict.

Comment 37

[Dominic Kuo [:dkuo]]

(In reply to sasikala from comment #35)
> Hi Dominic,
> Will you please let me know the commands for rebasing the code?
> 
> I have done the below commands:
> 1) git fetch upstream 
> 2) git rebase upstream/master
> 3) Fix the conflicts if any. Not found any conflicts
> 4) git push -f origin YOUR_BRANCH_NAME
> 
> Let me know the commands. I will try rebasing once again if same issue
> happens i will raise a new PR and upload it.
> 
> Thanks!

The PR I mentioned is in https://github.com/mozilla-b2g/gaia/pull/23198 (attachment 8477365 [details])
and the last commit was 8 days ago, so I guess you didn't successfully force pushed to your branch, anyway, I will land it for you and will close this bug, thanks.

Comment 38

[Dominic Kuo [:dkuo]]

Let's hold this patch first because bug 1069135 is also discussing about the mimetype issue between sms and music/video, the solution might related to this patch, probably we could fix them in sms side without landing this patch.

Comment 39

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 40

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(