976172 - Enable Audio tunneling to DSP in Gonk

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[bhargavg1]

Tracking bug for enabling Audio tunneling to DSP for better power battery performance while doing music playback. 

High level overview of the feature from KK release notes can be found at, 
https://developer.android.com/about/versions/kitkat.html -> "Audio tunneling to DSP"

Comment 1

[bhargavg1]

We have the basic framework available, able to do play/pause and hope to complete seek & timer updates soon. Shall share the patch for feedback

Comment 2

[Sotaro Ikeda [:sotaro]]

It seems that when "Audio tunneling" is enabled, we can not use WebAudio. Is it correct? If yes, it seems that WebAudio and Audio tunneling can not coexist at the same time. So, it seems necessary to explicitly trigger "Audio tunneling" from a content side, otherwise, web api's consistency becomes broken.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> It seems that when "Audio tunneling" is enabled, we can not use WebAudio. Is
> it correct? If yes, it seems that WebAudio and Audio tunneling can not
> coexist at the same time. So, it seems necessary to explicitly trigger
> "Audio tunneling" from a content side, otherwise, web api's consistency
> becomes broken.

Rob - Can you weigh in here on Sotaro's concerns about enabling audio tunneling potentially causing problems for Web Audio?

Comment 4

[bhargavg1]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> It seems that when "Audio tunneling" is enabled, we can not use WebAudio. Is
> it correct? If yes, it seems that WebAudio and Audio tunneling can not
> coexist at the same time. So, it seems necessary to explicitly trigger
> "Audio tunneling" from a content side, otherwise, web api's consistency
> becomes broken.

the idea is while playing audio files using OMX decoder, we make a check if data can be offloaded if so invoke the offloadplayer else fallback to the original path

Comment 5

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

I think that's right.

Ideally, audio tunneling would be automatically enabled for <audio> elements that are playing to speakers and aren't connected to Web Audio or MediaStreams. As soon as the element is connected to WebAudio via MediaElementAudioSourceNode, or connected to MediaStreams via mozCaptureStream(UntilEnded), it should stop going through the audio tunneling path. That may be hard to do without glitching but I think that's probably OK since normally those connections would be set up before playback begins.

Comment 6

[vasanth]

Attachment: 0001-Preliminary-WIP-Audio-Tunneling-Offloading-from-Geck.patch

1. Gecko Audio tunnel mode WIP, where play and pause works.
2. It has stripped version of Android AudioPlayer.cpp, MediaPlayerInterface::AudioSink, MediaPlayerService::AudioOutput.
3. When it is possible to offload audio, it tries to stop state machine and start play back through AudioOffloadPlayer (stripped android AudioPlayer)

Comment 7

[vasanth]

Comment on attachment 8382223 [details] [diff] [review]
0001-Preliminary-WIP-Audio-Tunneling-Offloading-from-Geck.patch

Verified on top of

Gecko sha1: d200d825bd94bf3cde4ab1555fed54909fd22449
Bug 959505 - Porting GonkNativeWindowClient, GonkBufferQueue, and FakeSurfaceComposer

Gaia sha1: e094c178b93e40dcdb4564c155e11ad45c69b04c
Bug 963225 - JavascriptException: TypeError: this._iccCard is null r=jaoo

Comment 8

[vasanth]

Comment on attachment 8382223 [details] [diff] [review]
0001-Preliminary-WIP-Audio-Tunneling-Offloading-from-Geck.patch

Ouch. Above sha1s are invalid

Gecko: fdde61f35486f5b68931262e1134ce4bff71b120
Gaia: ac8a273809b5b160554e616bc5ef2d6fa026ce0e

Comment 9

[bhargavg1]

Just wanted to give a brief overview about the changes,

- Current plan is to try offloading only for non-realtime audio streams only (no a/v) 
- check for offload support from platform for the mimetype through canOffload [1]
- Need to keep the omxcodec route open, when we connect BT headset or receive event CB_EVENT_TEAR_DOWN, fallback to the s/w codec path

This is more similar to what we have currently in platform

AudioPlayer handling of offload [2]
AwesomePlayer deciding which mode to use [3]

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/include/media/stagefright/Utils.h?h=b2g_kk_3.5#n60
[2] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/AudioPlayer.cpp?h=b2g_kk_3.5#n
[3] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/AwesomePlayer.cpp?h=b2g_kk_3.5#n1599

Comment 10

[Preeti Raghunath(:Preeti)]

Roc/ Sotaro,

Once we have a patch from QC, can one of you please review the patch?

Comment 11

[Sotaro Ikeda [:sotaro]]

About the audio :padenot ([e-mail]) seems correct person to review the patch than me.

Comment 12

[Sotaro Ikeda [:sotaro]]

Changed to correct component.

Comment 13

[Preeti Raghunath(:Preeti)]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> About the audio :padenot ([e-mail]) seems correct person to review the
> patch than me.

Thanks Sotaro.

Paul,

Can you please help with the review of the patch once QC has one.

Comment 14

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

(In reply to Preeti Raghunath(:Preeti) from comment #10)
> Roc/ Sotaro,
> 
> Once we have a patch from QC, can one of you please review the patch?

Of course.

Comment 15

[Paul Adenot (:padenot)]

(In reply to Preeti Raghunath(:Preeti) from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] from comment #11)
> > About the audio :padenot ([e-mail]) seems correct person to review the
> > patch than me.
> 
> Thanks Sotaro.
> 
> Paul,
> 
> Can you please help with the review of the patch once QC has one.

Yes, I'll be happy to have a look.

Comment 16

[Sandip Kamat]

PM Triage: 1.4+

Comment 17

[bhargavg1]

(In reply to Paul Adenot (:padenot) from comment #15)
> (In reply to Preeti Raghunath(:Preeti) from comment #13)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #11)
> > > About the audio :padenot ([e-mail]) seems correct person to review the
> > > patch than me.
> > 
> > Thanks Sotaro.
> > 
> > Paul,
> > 
> > Can you please help with the review of the patch once QC has one.
> 
> Yes, I'll be happy to have a look.

Please note we have an initial WIP patch to give a brief overview of the changes being done. Should be able to upload new one by early next week

Comment 18

[vasanth]

Attachment: 0001-Audio-Offloading-from-Gecko-WIP-DONOTMERGEYet.patch

Attached patch implements audio offloading/tunneling from gecko.
With this patch, we see considerable power savings for AAC/MP3 playback usecase.

Gecko commit base
f60c92c019cd9fe32eb24e4a1221367b739a05ba
Backed out changeset f94ee00aa4d6 (bug 974197) for causing gaia-ui-test failures

How it works
------------
1. In MediaOmxReader::ReadMetadata(), check weather audio can be offloaded 
(Pre requisites - No video, No streaming, audio.offload.disable property should be 0)
2. After MediaDecoder::MetadataLoaded(), try to offlaod audio if #1 returned true
3. If #2 is success, pause state machine using MediaDecoderStateMachine::SetDormant(true)
4. Initialize AudioOffloadPlayer, by using AudioTrack as its Source and also create 
an AudioOutput (AudioSink wrapper over AudioTrack similar to Android)
5. Pass MediaDecoder state changes to AudioOffloadPlayer and run accordingly
6. Pass Playback position changed, Playback Ended (End of Stream), Audio Teardown (when BT A2DP headset is connected) events
from AudioOffloadPlayer to MediaDecoder similar to state machine
7. In reception to Audio Teardown event (offloading cannot be continued), 
switch back to state machine using MediaDecoderStateMachine::SetDormant(false)
8. If #1 or #2 is failed, continue using state machine


Issues
-------
1. After receiving Audio Teardown, if BT headset is disconnected, 
not switching back to offloading for current stream. (We are ok with this for now)
2. During last ~7 seconds of offload playback, unable to pause->play. (Debugging. will fix it in next patch)

Comment 19

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

Comment on attachment 8389223 [details] [diff] [review]
0001-Audio-Offloading-from-Gecko-WIP-DONOTMERGEYet.patch

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

Some initial comments:

It would be good to split this patch up into individually buildable patches. Probably the OMX changes should be in one patch and the cross-platform patches should be in another patch.

We'll need to add something to handle the case where the media element output is going to a MediaStream (including Web Audio).

How are pause/play/seek/playbackRate supposed to work with this patch? It seems like we probably need to support switching in and out of the offload mode. I assume that will glitch but that's OK in this case I think.

::: content/media/AbstractMediaDecoder.h
@@ +112,5 @@
>    // on the main thread.
>    virtual MediaDecoderOwner* GetOwner() = 0;
>  
> +#ifdef MOZ_AUDIO_OFFLOAD
> +  virtual void SetCanOffloadAudio(bool aCanOffloadAudio) {};

This is really "SetAudioOffloaded" I think.

::: content/media/omx/AudioOffloadPlayer.cpp
@@ +33,5 @@
> +
> +#define LOG_TAG "AudioOffloadPlayer"
> +
> +// Number of milliseconds between timeupdate events as defined by spec
> +#define TIMEUPDATE_MS 250

We should find a way to share the existing definition instead of defining this again here.

::: content/media/omx/AudioOffloadPlayer.h
@@ +40,5 @@
> +namespace mozilla {
> +
> +class MediaOmxDecoder;
> +
> +class AudioOffloadPlayer {

This class, and its methods, really need some documentation in comments.

::: content/media/omx/AudioOutput.cpp
@@ +205,5 @@
> +
> +void AudioOutput::stop()
> +{
> +  AUDIO_OFFLOAD_LOG(PR_LOG_DEBUG, ("%s", __PRETTY_FUNCTION__));
> +  if (mTrack != 0) mTrack->stop();

In Gecko we write this like this:

  if (mTrack != 0) {
    mTrack->stop();
  }

::: content/media/omx/AudioOutput.h
@@ +25,5 @@
> +#include <AudioTrack.h>
> +
> +#include "AudioSink.h"
> +
> +class AudioOutput : public AudioSink

This class needs documentation too

Comment 20

[vasanth]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Comment on attachment 8389223 [details] [diff] [review]
> It would be good to split this patch up into individually buildable patches.

Will come up with separate patches and added documentation

> How are pause/play/seek/playbackRate supposed to work with this patch?

Verified pause/play/seek works in offload mode and also in normal mode if we switch from offload -> normal. Switching normal -> offload is not supported for same stream as mentioned in issue #1.
playbackrate change is also not supported yet. Will see if we can add that for offloaded track

Comment 21

[vasanth]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> How are pause/play/seek/playbackRate supposed to work with this patch? 

Oh. Are you asking how it works? We added MediaOmxDecoder::ChangeState() where we check whether audio is offloaded, then pass the play state changes to AudioOffloadPlayer::ChangeState() which calls corresponding functions to play, pause or seek the offloaded audio track

Comment 22

[vasanth]

Attachment: 0001-Audio-Offload-1-OMX-MediaDecoder-changes.patch

Added some documentation in header files, fixed code style and removed unused functions

Comment 23

[vasanth]

Attachment: 0001-Audio-Offload-2-Enable-audio-offloading-in-gecko.patch

Comment 24

[vasanth]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> We'll need to add something to handle the case where the media element
> output is going to a MediaStream (including Web Audio).

1. Could you please let us know more on how MediaStream makes use of MediaDecoder so that we can add a check to not offload Web Audio streams?

> How are pause/play/seek/playbackRate supposed to work with this patch? It

2. Seems PlaybackRate change is not supported by DSPs as well as offloaded audio tracks [1]
We can add a check to not offload if playback rate is not 1.0 or switch out of offload mode
when playback rate changes. Will that be ok?
We are assuming is playbackrate won't change for locally played files from Music app. So we are ok with this

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libmedia/AudioTrack.cpp?h=kk_3.5#n639

> > +  virtual void SetCanOffloadAudio(bool aCanOffloadAudio) {};
> 
> This is really "SetAudioOffloaded" I think.

3. That was intentional. First we check whether we "can" offload audio, then we try to offload and only when we create offloaded audio track, we SetAudioOffloaded. Removed "set" to make it more clear.

4. Have tried to address other comments. Please review

Comment 25

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

(In reply to vasanth from comment #24)
> 2. Seems PlaybackRate change is not supported by DSPs as well as offloaded
> audio tracks [1]
> We can add a check to not offload if playback rate is not 1.0 or switch out
> of offload mode
> when playback rate changes. Will that be ok?

Yes.

> 3. That was intentional. First we check whether we "can" offload audio, then
> we try to offload and only when we create offloaded audio track, we
> SetAudioOffloaded. Removed "set" to make it more clear.

No; given your explanation SetCanOffloadAudio is the correct name :-).

Comment 26

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

Comment on attachment 8390614 [details] [diff] [review]
0001-Audio-Offload-1-OMX-MediaDecoder-changes.patch

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

You need to replace this commit message with a proper message explaining what the patch does.

I think you should just remove the MOZ_AUDIO_OFFLOAD #ifdef.

::: content/media/AbstractMediaDecoder.h
@@ +112,5 @@
>    // on the main thread.
>    virtual MediaDecoderOwner* GetOwner() = 0;
>  
> +#ifdef MOZ_AUDIO_OFFLOAD
> +  virtual void CanOffloadAudio(bool aCanOffloadAudio) {};

Change this back to GetCanOffloadAudio

@@ +116,5 @@
> +  virtual void CanOffloadAudio(bool aCanOffloadAudio) {};
> +
> +  virtual void SetElementVisibility(bool aIsVisible) {};
> +
> +  virtual bool IsAudioOffloaded() { return false; }

Document all three of these methods.

::: content/media/MediaDecoderStateMachine.h
@@ +362,4 @@
>  
>    void AssertCurrentThreadInMonitor() const { mDecoder->GetReentrantMonitor().AssertCurrentThreadIn(); }
>  
> +  nsRefPtr<MediaDecoder> GetDecoder() { return mDecoder; }

just return MediaDecoder*

Comment 27

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

Comment on attachment 8390614 [details] [diff] [review]
0001-Audio-Offload-1-OMX-MediaDecoder-changes.patch

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

It seems to me the patches are in the wrong order.

Paul doesn't need to review this patch. He can review the other patch (and I don't need to review that one).

If the MOZ_AUDIO_OFFLOAD #ifdef is needed to avoid build failures on some platforms, at least minimize the use of it to AudioOffloadPlayer.cpp.

It looks like patches are in the wrong order. Patch 2 is needed to build this patch.

Overall it looks reasonable. I like the way you're using the existing SetDormant infrastructure to pause and resume the MediaDecoderStateMachine.

::: content/media/MediaDecoderStateMachine.h
@@ +367,5 @@
> +  // The decoder monitor must be obtained before modifying this state.
> +  // NotifyAll on the monitor must be called when the state is changed so
> +  // that interested threads can wake up and alter behaviour if appropriate
> +  // Accessed on state machine, audio, main, and AV thread.
> +  State mState;

Why have you moved this? Better to add an IsDormant method.

::: content/media/omx/Makefile.in
@@ +19,4 @@
>  		-I$(ANDROID_SOURCE)/frameworks/native/opengl/include/ \
>  		-I$(ANDROID_SOURCE)/frameworks/native/include/ \
>  		-I$(ANDROID_SOURCE)/frameworks/av/include/media/ \
> +		-I$(ANDROID_SOURCE)/hardware/libhardware/include/ \

This file doesn't exist anymore

::: content/media/omx/MediaOmxDecoder.cpp
@@ +58,5 @@
> +
> +MediaOmxDecoder::~MediaOmxDecoder() {
> +  if (mAudioOffloadPlayer) {
> +    delete mAudioOffloadPlayer;
> +    mAudioOffloadPlayer = nullptr;

Make mAudioOffloadPlayer an nsAutoPtr<AudioOffloadPlayer> so we don't need explicit delete. In fact you won't need an explicit destructor at all.

@@ +64,5 @@
> +}
> +
> +void MediaOmxDecoder::MetadataLoaded(int aChannels, int aRate, bool aHasAudio,
> +    bool aHasVideo, MetadataTags* aTags) {
> +  MediaDecoder::MetadataLoaded(aChannels, aRate, aHasAudio, aHasVideo, aTags);

Please add assertions in this method and all other methods of MediaOmxDecoder to check you're on the right thread.

@@ +84,5 @@
> +      }
> +    }
> +    // Call ChangeState() to run AudioOffloadPlayer since offload state enabled
> +    ChangeState(mPlayState);
> +

Move ChangePlayState into the err == OK path and return after it. Then you don't need gotos.

@@ +105,5 @@
> +void MediaOmxDecoder::SetDormantIfNecessary(bool aDormant)
> +{
> +  if (!mAudioOffloaded) {
> +    MediaDecoder::SetDormantIfNecessary(aDormant);
> +  }

I don't think you should override this function. OmxReader::IsDormantNeeded should always return false if the audio is offloaded, since it only returns true if there's a video track.

@@ +114,5 @@
> +  if (!mDecoderStateMachine) {
> +    return;
> +  }
> +  StopProgress();
> +  DestroyDecodedStream();

I don't think you should call DestroyDecodedStream here.

@@ +153,5 @@
> +
> +void MediaOmxDecoder::ApplyStateToStateMachine(PlayState aState) {
> +  if (!mAudioOffloaded) {
> +    MediaDecoder::ApplyStateToStateMachine(aState);
> +  }

Why are you changing this method?

@@ +161,3 @@
>  {
> +  if (!mAudioOffloaded) {
> +    MediaDecoder::PlaybackPositionChanged();

Just return immediately after this so the following code in this function doesn't have to be indented.

Similar in other functions in this file.

@@ +183,5 @@
> +}
> +
> +void MediaOmxDecoder::NotifyPlaybackStopped() {
> +  if (!mAudioOffloaded) {
> +    MediaDecoder::NotifyPlaybackStopped();

Why are we overriding this method?

::: content/media/omx/MediaOmxDecoder.h
@@ +44,5 @@
> +  }
> +  virtual bool IsAudioOffloaded() { return mAudioOffloaded; }
> +  void AudioOffloadTearDown();
> +  double GetSeekTime() { return mRequestedSeekTime; }
> +  void ResetSeekTime() { mRequestedSeekTime = -1.0; }

Where are these methods used?

I don't see how seeking is handled in this patch.

@@ +47,5 @@
> +  double GetSeekTime() { return mRequestedSeekTime; }
> +  void ResetSeekTime() { mRequestedSeekTime = -1.0; }
> +
> +private:
> +  bool mAudioOffloaded;

Can we remove this variable and just make IsAudioOffloaded test mAudioOffloadPlayer?

@@ +49,5 @@
> +
> +private:
> +  bool mAudioOffloaded;
> +  bool mCanOffloadAudio;
> +  bool mAudioTearDownReceived;

Document these

@@ +53,5 @@
> +  bool mAudioTearDownReceived;
> +
> +  sp<MediaSource> mAudioTrack;
> +  MediaOmxReader *mReader;
> +  AudioOffloadPlayer *mAudioOffloadPlayer;

Document these. Also, for all the variables introduced here, indicate which threads are allowed to access them and if multiple threads can access them, how that access is synchronized.

::: content/media/omx/MediaOmxReader.cpp
@@ +402,5 @@
> +  char offloadProp[128];
> +  property_get("audio.offload.disable", offloadProp, "0");
> +  bool offloadDisable =  atoi(offloadProp) != 0;
> +
> +  if (!offloadDisable) {

Just do
  if (offloadDisable) {
    return;
  }

@@ +409,5 @@
> +        ? mAudioOffloadTrack->getFormat() : nullptr;
> +    bool hasNoVideo = !mOmxDecoder->HasVideoSource();
> +    bool isNotStreaming
> +        = mDecoder->GetResource()->IsDataCachedToEndOfResource(0);
> +    bool isTypeMusic = mAudioChannelType == dom::AUDIO_CHANNEL_CONTENT;

Is there a reason for this restriction? Seems to me it would be simpler to not check the channel type.

@@ +417,5 @@
> +        isNotStreaming, mAudioChannelType));
> +
> +    // Supporting audio offload only when there is no video and no streaming
> +    if ((meta != nullptr) && hasNoVideo && isNotStreaming && isTypeMusic
> +        && canOffloadStream(meta, false, false, AUDIO_STREAM_MUSIC)) {

Where is canOffloadStream defined?

Comment 28

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

(In reply to vasanth from comment #24)
> 1. Could you please let us know more on how MediaStream makes use of
> MediaDecoder so that we can add a check to not offload Web Audio streams?

MediaDecoder::AddOutputStream gets called. If there are any output streams, or AddOutputStream gets called, you need to stop (or not start) offloading audio output.

Comment 29

[vasanth]

Thanks Robert. Will address the comments.

We are waiting for comments in other patch as well. Adding ni

Comment 30

[Paul Adenot (:padenot)]

Comment on attachment 8390615 [details] [diff] [review]
0001-Audio-Offload-2-Enable-audio-offloading-in-gecko.patch

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

In general, looks good, r- mostly for style and the need for comments. I'm pretty sure having a second look with the style problems fixed and more explanation will be worthwhile.

It's a bit weird to have a mix of Gecko and Android style C++ in the same file. Could you harmonize the files to Gecko style, since this appear to be a fork of the files (not intended to track upstream)? If this is not the case, please ignore this comment.

https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style

::: content/media/omx/AudioOffloadPlayer.cpp
@@ +372,5 @@
> +    AudioSink *audioSink,
> +    void *buffer, size_t size, void *cookie,
> +    AudioSink::cb_event_t event) {
> +  AudioOffloadPlayer *me = (AudioOffloadPlayer *)cookie;
> +  nsCOMPtr<nsIRunnable> nsEvent;

nsEvent is unused.

@@ +535,5 @@
> +  if (mIsElementVisible) {
> +    AUDIO_OFFLOAD_LOG(PR_LOG_DEBUG, ("Element is visible. Start time update"));
> +    StartTimeUpdate();
> +  }
> +}

This is not compliant with HTML, but I think it's acceptable for what we are trying to achieve, here.

@@ +559,5 @@
> +    StopTimeUpdate();
> +  }
> +}
> +
> +nsresult AudioOffloadPlayer::StartTimeUpdate() {

Can you sprinkle a couple MOZ_ASSERT(NS_IsMainThread()) in the relevant functions ?

@@ +561,5 @@
> +}
> +
> +nsresult AudioOffloadPlayer::StartTimeUpdate() {
> +  if (mTimeUpdateTimer)
> +    return NS_OK;

Please always brace you if statements, in the whole patch.

::: content/media/omx/AudioOffloadPlayer.h
@@ +67,5 @@
> +
> +  virtual ~AudioOffloadPlayer();
> +
> +  // Caller retains ownership of "source".
> +  void SetSource(const sp<MediaSource> &source);

aSource, & near the type.

@@ +71,5 @@
> +  void SetSource(const sp<MediaSource> &source);
> +
> +  // Start the source if it's not already started and open the AudioSink to
> +  // create an offloaded audio track
> +  status_t Start(bool sourceAlreadyStarted = false);

aSourceAlreadyStarted

@@ +111,5 @@
> +  size_t mFrameSize;
> +  int64_t mStartPosUs;
> +  int64_t mSeekTimeUs;
> +  status_t mFinalStatus;
> +  int64_t mPositionTimeMediaUs;

Can you explain what those members are for? In particular, the seeking logic, and on which thread each member can be accessed (with the eventual locking) but it would be good to explain everything.

@@ +115,5 @@
> +  int64_t mPositionTimeMediaUs;
> +
> +  MediaDecoder::PlayState mPlayState;
> +
> +  android::Mutex mLock;

What members this lock is protecting?

@@ +120,5 @@
> +
> +  sp<MediaSource> mSource;
> +  sp<AudioSink> mAudioSink;
> +
> +  MediaBuffer *mInputBuffer;

* near the type.

@@ +174,5 @@
> +  // Offloaded audio track is invalidated due to usecase change. Notify
> +  // MediaDecoder to re-evaluate offloading options
> +  void NotifyAudioTearDown();
> +
> +  bool ReachedEOS(status_t *finalStatus);

Comment, please.

::: content/media/omx/AudioOutput.h
@@ +73,5 @@
> +
> +  float mLeftVolume;
> +  float mRightVolume;
> +
> +  // sample rate of the content, as set in open()

This comment should go a couple lines below, right?

Comment 31

[Paul Adenot (:padenot)]

(clearing needinfo)

Comment 32

[vasanth]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> If the MOZ_AUDIO_OFFLOAD #ifdef is needed to avoid build failures on some
> platforms, at least minimize the use of it to AudioOffloadPlayer.cpp.

If we remove MOZ_AUDIO_OFFLOAD from MediaOmxDecoder, then AudioOffloadPlayer code has to get compiled in platforms wherever MEDIA_OMX_DECODER is used. I hope that's ok?

Comment 33

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

Yes, that should be fine.

Comment 34

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

As long as it builds on our existing Android/B2G platforms of course.

Comment 35

[bhargavg1]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> As long as it builds on our existing Android/B2G platforms of course.

also note that this has dependency on underlying android platform, it has to be KK at least

Comment 36

[vasanth]

Attachment: V3-part1-Changes-in-MediaOmxDecoder-to-enable-audio-offloadin.patch

Comment 37

[vasanth]

Attachment: V3-part2-Enable-audio-offloading-in-gecko.patch

Comment 38

[vasanth]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> If the MOZ_AUDIO_OFFLOAD #ifdef is needed to avoid build failures on some
> platforms, at least minimize the use of it to AudioOffloadPlayer.cpp.

It's needed to avoid build failures when ANDROID_VERSION < 19 (KK) is used
Tried to minimize it by introducing a new base class AudioOffloadPlayerBase
to use in MediaOmxDecoder class without depending on libStageFright functions
Comments please.

> It looks like patches are in the wrong order. Patch 2 is needed to build
> this patch.

Thats intentional. First patch just adds changes to MediaOmxDecoder, can be
built without errors. But Audio offloading functionality won't be enabled
unless you have 2nd. Even if I change the order, 1st patch alone cannot 
enable offloading?

> 
> > +void MediaOmxDecoder::ApplyStateToStateMachine(PlayState aState) {
> > +  if (!mAudioOffloaded) {
> > +    MediaDecoder::ApplyStateToStateMachine(aState);
> > +  }
> 
> Why are you changing this method?

During offload playback, state machine should be in dormant state.
ApplyStateToStateMachine() can change state machine state to
something else or reset the seek time. So don't call this when audio is
offloaded
Added this comment in code as well

> > +  double GetSeekTime() { return mRequestedSeekTime; }
> > +  void ResetSeekTime() { mRequestedSeekTime = -1.0; }
> 
> Where are these methods used?
Used by AudioOffloadPlayer. Else we should make mRequestedSeekTime public


> I don't see how seeking is handled in this patch.
AudioOffloadPlayer can send SeekingStarted and SeekingStopped events
back to MediaDecoder. Default MediaDecoder version of those functions
works for me. Also added how seek works in AudioOffloadPlayer::SeekTo()

> > +    bool isTypeMusic = mAudioChannelType == dom::AUDIO_CHANNEL_CONTENT;
> 
> Is there a reason for this restriction? Seems to me it would be simpler to
> not check the channel type.
Yes. There is not much benefit in trying for channel type. Most of them won't
be supported and also duration will be less than 1min.

> Where is canOffloadStream defined?
It's in libStageFright Utils.cpp That internally checks whether underlying 
platform can support offloading

(In reply to Paul Adenot (:padenot) from comment #30)

> https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style
One thing not clear to me from the link is. indentation if function def is longer 
than 80 chars. 
Have tried addressing all other coding style requirements

Comment 39

[bhargavg1]

(In reply to vasanth from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> > If the MOZ_AUDIO_OFFLOAD #ifdef is needed to avoid build failures on some
> > platforms, at least minimize the use of it to AudioOffloadPlayer.cpp.
> 
> It's needed to avoid build failures when ANDROID_VERSION < 19 (KK) is used
> Tried to minimize it by introducing a new base class AudioOffloadPlayerBase
> to use in MediaOmxDecoder class without depending on libStageFright functions
> Comments please.
> 

I am inclining to use the existing macro ANDROID_VERSION for version support check, don't think its really necessary to add more defs

Comment 40

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

(In reply to bhargavg1 from comment #39)
> I am inclining to use the existing macro ANDROID_VERSION for version support
> check, don't think its really necessary to add more defs

Yes, that seems like a good idea.

Comment 41

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

Comment on attachment 8393658 [details] [diff] [review]
V3-part1-Changes-in-MediaOmxDecoder-to-enable-audio-offloadin.patch

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

Please remove the metadata lines and the git changeset ID from the commit comment.

r+ with those changes.

Note, we definitely need automated tests for this feature. It will be easy to accidentally break it with HTML media element changes.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3415,5 @@
>  
>    if (mDecoder) {
> +#ifdef MOZ_AUDIO_OFFLOAD
> +    mDecoder->SetElementVisibility(!ownerDoc->Hidden());
> +#endif

#ifdef not needed here

::: content/media/omx/MediaOmxDecoder.cpp
@@ +155,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  PlaybackPositionChanged();
> +
> +  if (mAudioOffloadPlayer && ((aPlaybackRate != 0.0) ||
> +      (aPlaybackRate != 1.0))) {

Put the second operand of && on its own line

::: content/media/omx/MediaOmxDecoder.h
@@ +54,5 @@
> +
> +  // Offloaded audio track
> +  android::sp<MediaSource> mAudioTrack;
> +
> +  nsAutoPtr<AudioOffloadPlayerBase> mAudioOffloadPlayer;

Reorder the fields so that pointers (including smart pointers) go first, followed by the bools. Also, move all the fields down below the last methods.

::: content/media/omx/MediaOmxReader.cpp
@@ +417,5 @@
> +
> +  bool hasNoVideo = !mOmxDecoder->HasVideo();
> +  bool isNotStreaming
> +      = mDecoder->GetResource()->IsDataCachedToEndOfResource(0);
> +  bool isTypeMusic = mAudioChannelType == dom::AUDIO_CHANNEL_CONTENT;

Please at least document why this check is here.

Comment 42

[Paul Adenot (:padenot)]

Comment on attachment 8393659 [details] [diff] [review]
V3-part2-Enable-audio-offloading-in-gecko.patch

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

Looks good, thanks !

::: content/media/omx/AudioOffloadPlayer.cpp
@@ +310,5 @@
> +  mAudioSink->Flush();
> +
> +  if (mPlaying) {
> +    mAudioSink->Start();
> +

nit: remove this blank line.

@@ +500,5 @@
> +            AUDIO_OFFLOAD_LOG(PR_LOG_DEBUG, ("FillBuffer posting SEEK_COMPLETE"));
> +            nsCOMPtr<nsIRunnable> nsEvent = NS_NewRunnableMethod(mObserver,
> +                &MediaDecoder::SeekingStopped);
> +            NS_DispatchToMainThread(nsEvent, NS_DISPATCH_NORMAL);
> +

nit: blank line.

::: content/media/omx/AudioOffloadPlayer.h
@@ +72,5 @@
> +  AudioOffloadPlayer(MediaOmxDecoder* aDecoder = nullptr);
> +
> +  ~AudioOffloadPlayer();
> +
> +  // Caller retains ownership of "source".

nit: s/source/aSource/

Comment 43

[vasanth]

Thanks Paul and Robert! 
Will address nits, use ANDROID_VERSION and upload hg friendly patches for checkin

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Note, we definitely need automated tests for this feature. It will be easy
> to accidentally break it with HTML media element changes.

Sure Robert. Please point me to music test scripts you guys run everyday, I will modify them to include tunnel mode clips and see if I can add some more scenarios to test this feature in few days. Also I guess that can be a separate changeset and we can merge these for now?

Comment 44

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

It would be good to push this through try before landing too.  I don't think there were any kk builds in try yet, but at least we can ensure that we won't burn ics or jb builds here (err, do we even have a jb build in try yet?)

Comment 45

[Paul Adenot (:padenot)]

We have KK and JB, on emulator. I've also added a regular linux as a sanity check, and then asked for mochitests 1 and 3 because they contain tests with mp3 playback (1 on desktop, 3 on mobile). Let me know if you need something else.

https://tbpl.mozilla.org/?tree=Try&rev=411d5484548d

Comment 46

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

Thanks Paul.  Looks like that try run is pretty red!

Comment 47

[vasanth]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #46)
> Thanks Paul.  Looks like that try run is pretty red!

Seems "B2G KK Emulator Opt" build and "Linux x64 Debug" test failures are because of some other intermittent failures. starred them.
Will analyze/fix "B2G ICS Emulator" build failures and update my chageset

Comment 48

[vasanth]

Attachment: Bug976172-Part1-Changes-in-MediaOmxDecoder.patch

Fixed the Try ICS emulator build errors and addressed nits

Comment 49

[vasanth]

Attachment: Bug976172-Part2-Enable-audio-offloading.patch

Comment 50

[vasanth]

Attachment: Bug976172-Part1-Changes-in-MediaOmxDecoder.patch

Comment 51

[vasanth]

Paul & Robert, 
Could one of you please push these 2 patches to try once?

Comment 52

[vasanth]

Seems V1.4 and master branch of gecko diverged a little bit. Will upload patches for V1.4 soon

One more thing is patch1 uses canOffloadStream() from stagefright utils. 
AOSP has 4 params for this function whereas latest caf kk_3.5  has 5 params. In the patch using AOSP's canOffloadStream() (4 params)
Will add a patch in device/qcom/b2g-common/patches/ to convert it to 5 params, so that it can work with both caf and aosp stagefright.

Comment 53

[Paul Adenot (:padenot)]

https://tbpl.mozilla.org/?tree=Try&rev=bff820c4b0ad

Comment 54

[bhargavg1]

we need to have further testing on this, please do not land until requested

Comment 55

[Jason Smith [:jsmith]]

We're past FC, so this might need to wait until 1.5, as this is a feature request. Renoming for more discussion.

Comment 56

[Inder]

Low Power Audio (LPA) is a required feature for QC FC and without this feature its not going to meet the power and perf goals so it's absolutely needed to be enabled in v1.4. This feature was approved for v1.4 QC FC since the beginning. All the other OS on this target has this feature enabled except FxOS.

Comment 57

[vasanth]

Attachment: Bug976172-V1.4-Part-1-Changes-in-MediaOmxDecoder-to-enab.patch

Patch 1 for V1.4

Comment 58

[vasanth]

Attachment: Bug976172-V1.4-Part2-Enable-audio-offloading.patch

Patch 2 for V1.4

Comment 59

[vasanth]

Hi Paul, 

Sorry for the delay. Uploaded 2 patches for V1.4.
Could you give a "Try" run and if it is success, we can go ahead and merge it.

Seems master diverged a little bit. Will upload patch 1 for master once V1.4 patches are merged.

Comment 60

[Paul Adenot (:padenot)]

https://tbpl.mozilla.org/?tree=Try&rev=2587394ae402, pushed on top of 1.4 branch (aurora).

Comment 61

[vasanth]

Thanks Paul. 
Seems only failure, Linux x64 Opt compile errors is totally unrelated to these patches.
Shall we go ahead and merge this?

I will upload patches for master soon.

Comment 62

[Paul Adenot (:padenot)]

Yes, we can land those on 1.4.

Comment 63

[Paul Adenot (:padenot)]

I got informed by our sheriff that this will not land in 1.4 then in 1.5. We need to get the patches and land for 1.5, then the right patches will get uplifted to 1.4.

Comment 64

[vasanth]

(In reply to Paul Adenot (:padenot) from comment #63)
> I got informed by our sheriff that this will not land in 1.4 then in 1.5. We
> need to get the patches and land for 1.5, then the right patches will get
> uplifted to 1.4.

Looks like in master, Bug 778077 removes (double) Mediadecoder::mRequestedSeekTime and introduces Mediadecoder::mRequestedSeekTarget which has (int64_t) mTime. Audio offload patches uses mRequestedSeekTime in MediaOmxDecoder which needs to be modified to use mRequestedSeekTarget.

We can do that which will take sometime.

Comment 65

[vasanth]

Attachment: Bug976172-master-Part-1-Changes-in-MediaOmxDecoder.patch

Patch 1 for master

Comment 66

[vasanth]

Attachment: Bug976172-master-Part2-Enable-audio-offloading.patch

Patch 2 for master

Comment 67

[vasanth]

(In reply to Paul Adenot (:padenot) from comment #63)
> I got informed by our sheriff that this will not land in 1.4 then in 1.5. We
> need to get the patches and land for 1.5, then the right patches will get
> uplifted to 1.4.

Paul, I modified my master patch 1 to make use mRequestedSeekTarget and the change seems trivial.
Could you please give a try run on master as well and we can merge in both master and V1.4?

Comment 68

[Paul Adenot (:padenot)]

https://tbpl.mozilla.org/?tree=Try&rev=98755530c12e, new patches on top of current mozilla-inbound.

Comment 69

[vasanth]

Thanks Paul. Among 6 errors, starred 1 of them and remaining 5 errors are same [1] and not sure how it is related to these patches. In Linux and B2G ICS platforms, most part of offload code won't get compiled at all.

I guess something else in current mozilla-bound could have caused the below error?

[1] "uncaught exception - ReferenceError: assignment to undeclared variable source at http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:824"

Comment 70

[Paul Adenot (:padenot)]

Yeah, I just saw that. I _think_ we are fine here, but I'll ask confirmation to people knowledgeable about those tests when they wake up. Then, we can certainly land if they tell us it's harmless.

It might very well be that we were unfortunate enough that the base revision for this try push was buggy.

Comment 71

[vasanth]

(In reply to Paul Adenot (:padenot) from comment #70)
> Then, we can certainly land if they tell us it's harmless.

Thanks again! Adding ni as usual.

Comment 72

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1904c970632
https://hg.mozilla.org/integration/mozilla-inbound/rev/6656fd9ae642

Comment 73

[Paul Adenot (:padenot)]

The two patches that need checkin on aurora are labeled as 1.4 in the attachment list.

Comment 74

[vasanth]

Hi Ryan, 
Would like to know why "checkin-needed-aurora" is removed from whiteboard?
Have you landed the patches in V1.4 (aurora)?
As per comment 56, this change is needed in 1.4 to meet our power goals.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Because I query for fixed 1.4 blockers needing uplift anyway and it was cluttering up my checkin-needed queries.

Also, feel free to ping me on IRC next time you have questions instead of needinfo? and CCing me to the bug.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1904c970632
https://hg.mozilla.org/mozilla-central/rev/6656fd9ae642

Comment 77

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

\o/

Comment 78

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91e8b45c5f5e
https://hg.mozilla.org/releases/mozilla-aurora/rev/f989ff4e46cd

Comment 79

[vasanth]

Thanks Robert, Paul & Ryan for your support!
I will come up with a test script for this feature in other bugzilla soon.

Comment 80

[[:RJ]]

Not enough information with current STR to write test case to address bug.