Closed Bug 1245052 Opened 8 years ago Closed 8 years ago

bug 1243608 broke b2g builds


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

45 Branch



Tracking Status
firefox47 --- fixed


(Reporter: fabrice, Assigned: fabrice)




(1 file)

      No description provided.
Attachment #8714731 - Flags: review?(jyavenard)
Comment on attachment 8714731 [details] [diff] [review]

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 :

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

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]

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 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.