1245052 - bug 1243608 broke b2g builds

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: mediadecoder.patch

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8714731 [details] [diff] [review]
mediadecoder.patch

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

r=me without the changes to MediaDecoder.{cpp,h}

::: dom/media/MediaDecoder.cpp
@@ +821,5 @@
>    return NS_OK;
>  }
>  
>  void
> +MediaDecoder::CallSeek(SeekTarget aTarget)

no, this can be const SeekTarget& (and should be).

We only pass the target by value in other areas due to the use of InvokeAsync and the way it uses templates, which causes problem when the variable goes out of scope.

It's out of scope for this patch anyway.

::: dom/media/MediaDecoder.h
@@ +604,5 @@
>    RefPtr<CDMProxyPromise> mCDMProxyPromise;
>  #endif
>  
>  protected:
> +  virtual void CallSeek(SeekTarget aTarget);

don't change that, leave it as is.

Comment 3

[[:fabrice] Fabrice Desré]

Well, if I revert my changes to MediaDecoder.{h|cpp} we still get :

Unified_cpp_dom_media2.o
In file included from ../../dist/include/MediaOmxDecoder.h:9:0,
                 from ../../../../mozilla-inbound/dom/media/DecoderTraits.cpp:32:
../../dist/include/MediaOmxCommonDecoder.h:29:8: error: 'void mozilla::MediaOmxCommonDecoder::CallSeek(mozilla::SeekTarget)' marked override, but does not override
In file included from ../../../../mozilla-inbound/dom/media/DecoderTraits.cpp:8:0:
../../../../mozilla-inbound/dom/media/MediaDecoder.h:608:16: error: 'virtual void mozilla::MediaDecoder::CallSeek(const mozilla::SeekTarget&)' was hidden [-Werror=overloaded-virtual]
In file included from ../../dist/include/MediaOmxDecoder.h:9:0,
                 from ../../../../mozilla-inbound/dom/media/DecoderTraits.cpp:32:
../../dist/include/MediaOmxCommonDecoder.h:29:8: error:   by 'void mozilla::MediaOmxCommonDecoder::CallSeek(mozilla::SeekTarget)' [-Werror=overloaded-virtual]
cc1plus: all warnings being treated as errors

Comment 4

[Jean-Yves Avenard [:jya]]

Oh I see, then make the change to MediaMoxCommonDecoder::CallSeek back to what it used to be before https://hg.mozilla.org/integration/mozilla-inbound/rev/2008d4a61715

I incorrectly changed it (I did so due to the race issues I described earlier), i was a bit too quick in copy past :(

should be just a matter of re-adding const SeekTarget& back to OMXMediaCommonDecoder.{cpp,h}
that is:
void MediaOmxCommonDecoder::CallSeek(const SeekTarget& aTarget)

It is also safe to use RefPtr<MediaDecoder::SeekPromise> AudioOffloadPlayer::Seek(const SeekTarget& aTarget)


thank you, and sorry for causing those problems.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f5434bbc37

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8714731 [details] [diff] [review]
mediadecoder.patch

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

::: dom/media/omx/AudioOffloadPlayer.cpp
@@ +345,5 @@
>    MOZ_ASSERT(mSeekTarget.IsValid());
>    CHECK(mAudioSink.get());
>  
>    AUDIO_OFFLOAD_LOG(LogLevel::Debug,
> +                    ("DoSeek ( %lld )", mSeekTarget.GetTime().ToMicroseconds()));

forgot to ask...

What are those parenthesis for ?

Doesn't this macro work without it ?

And when is OMX Reader supposed to go away?

Comment 7

[[:fabrice] Fabrice Desré]

The macro only takes two parameters, see https://mxr.mozilla.org/mozilla-central/source/dom/media/omx/AudioOffloadPlayer.cpp#46 and the uses in this file.

Why should OMX Reader go away?

Comment 8

[Jean-Yves Avenard [:jya]]

I thought I had seen a bug to remove it, following the new OMX MediaDataDecoder, at which point the OMX Reader will no longer be required.

Would allow transition to the new media architecture there too.

Comment 9

[Blake Wu [:bwu][:blakewu]]

Once we have AudioOffloadPlayer supported in Bug 1149984 under MediaFormatReader design for B2G, OMX Reader can go away.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8f5434bbc37