Closed Bug 1245052 Opened 4 years ago Closed 4 years ago

bug 1243608 broke b2g builds

Categories

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

45 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8714731 - Flags: review?(jyavenard)
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.
Attachment #8714731 - Flags: review?(jyavenard) → review+
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
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
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?
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?
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.
Once we have AudioOffloadPlayer supported in Bug 1149984 under MediaFormatReader design for B2G, OMX Reader can go away.
Blocks: 1245091
https://hg.mozilla.org/mozilla-central/rev/f8f5434bbc37
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.