Closed Bug 1175071 Opened 5 years ago Closed 5 years ago

[FFOS] cookpad.com videos fail to play

Categories

(Core :: Audio/Video, defect)

37 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bwu, Assigned: bwu)

Details

Attachments

(1 file, 2 obsolete files)

This problem was originally reported in Fennec in bug 1167647. 
After a quick check in bug 1167647 comment 12, B2G's Gonk PDM cannot play it either.
Omx decoder can decode AAC data without converting it to ADTS format. So I am going to un-use ADTS in Gonk PDM.
(In reply to Blake Wu [:bwu][:blakewu] from comment #1)
> Omx decoder can decode AAC data without converting it to ADTS format. So I
> am going to un-use ADTS in Gonk PDM.
jya, 
Can demuxer always output raw AAC data? If yes, I would like to configure decoder to take raw AAC data only.
Flags: needinfo?(jyavenard)
It already does. There's never adts coming out of the demuxer. It's the gonk decoder that convert it to adts.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> It already does. There's never adts coming out of the demuxer. It's the gonk
> decoder that convert it to adts.
Understood. I will remove it.
Per comment 1 and comment 3.
Attachment #8624028 - Flags: review?(jyavenard)
Comment on attachment 8624028 [details] [diff] [review]
Bug-1175071-Don-t-use-ADTS-format.patch

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

the code can be cleaned up and now unused code removed.

Or if you don't want do remove it now, then open a new bug removing the unused code with a link to here.
thanks.

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
@@ +38,4 @@
>    : mAudioChannels(aConfig.mChannels)
>    , mAudioRate(aConfig.mRate)
>    , mAudioProfile(aConfig.mProfile)
> +  , mUseAdts(false)

there is no use for mUseAdts anymore, all the code testing mUseAdts should be removed.

@@ +82,5 @@
>    format->setString("mime", mMimeType.get());
>    format->setInt32("channel-count", mAudioChannels);
>    format->setInt32("sample-rate", mAudioRate);
>    format->setInt32("aac-profile", mAudioProfile);
> +  format->setInt32("is-adts", false);

this should have been mUseAdts instead...

as before thise patch you set is-adts even for mp3 audio.
Attachment #8624028 - Flags: review?(jyavenard) → review-
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8624028 [details] [diff] [review]
> Bug-1175071-Don-t-use-ADTS-format.patch
> 
> Review of attachment 8624028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the code can be cleaned up and now unused code removed.
The reason I didn't remove all the codes related to ADTS is I think there might be one case that we may need to decode mpeg2-ts stream which is usually in ADTS, which Android sets ADTS in this case IIRC. If there is no such case and demuxer can always output raw AAC, there will be no reason to keep the unused codes. sorry! I should explain my thought in the first place. 
> 
> Or if you don't want do remove it now, then open a new bug removing the
> unused code with a link to here.
> thanks.
> 
> ::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
> @@ +38,4 @@
> >    : mAudioChannels(aConfig.mChannels)
> >    , mAudioRate(aConfig.mRate)
> >    , mAudioProfile(aConfig.mProfile)
> > +  , mUseAdts(false)
> 
> there is no use for mUseAdts anymore, all the code testing mUseAdts should
> be removed.
> 
> @@ +82,5 @@
> >    format->setString("mime", mMimeType.get());
> >    format->setInt32("channel-count", mAudioChannels);
> >    format->setInt32("sample-rate", mAudioRate);
> >    format->setInt32("aac-profile", mAudioProfile);
> > +  format->setInt32("is-adts", false);
> 
> this should have been mUseAdts instead...
> 
> as before thise patch you set is-adts even for mp3 audio.
This setting, is-adts, is only for AAC format. For mp3, it will be ignored. But it would be better to be set only for AAC case. I will modify it. Thanks for this reminder.
Per comment 6, this is the cleaned version to remove all codes related to ADTS.
Attachment #8624103 - Flags: review?(jyavenard)
Comment on attachment 8624103 [details] [diff] [review]
0001-Bug-1175071-Remove-those-codes-which-handles-ADTS.patch

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

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
@@ +84,1 @@
>    status_t err = mDecoder->configure(format, nullptr, nullptr, 0);

don't use have to pass is-adts, false ?
Attachment #8624103 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Comment on attachment 8624103 [details] [diff] [review]
> 0001-Bug-1175071-Remove-those-codes-which-handles-ADTS.patch
> 
> Review of attachment 8624103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
> @@ +84,1 @@
> >    status_t err = mDecoder->configure(format, nullptr, nullptr, 0);
> 
> don't use have to pass is-adts, false ?
No need. If "is-adts" is not set, omx codec(ACodec.cpp) will assume it is not ADTS format[1].  

[1]http://androidxref.com/4.4.4_r1/xref/frameworks/av/media/libstagefright/ACodec.cpp#1219
Carry r+ from jya.
Attachment #8624028 - Attachment is obsolete: true
Attachment #8624103 - Attachment is obsolete: true
Attachment #8624558 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8ce943a30f80
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.