1276570 - Split MediaDecoderReader::TargetQueues into bit-field flags

Status: RESOLVED FIXED

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

Description

[u480271]

Change VIDEO_ONLY and AUDIO_VIDEO into "or"-able bit-fields.

Comment 1

[u480271]

From review comments to bug 1276495:

I would prefer bit flags like:
enum TargetQueues {
  VIDEO = 0x1,
  AUDIO = 0x2
};

and code like:
if (flag & MediaDecoderReader::VIDEO) {
  // do something about video
}
if (flag & MediaDecoderReader::AUDIO) {
  // do something about audio
}

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Dan Glastonbury :kamidphish from comment #1)
> From review comments to bug 1276495:
> 
> I would prefer bit flags like:
> enum TargetQueues {
>   VIDEO = 0x1,
>   AUDIO = 0x2
> };

For more type safety:

EnumSet<TargetQueues> flags;

if (flag.contains(MediaDecoderReader::VIDEO)) {
  // do something about video
}
if (flag.contains(MediaDecoderReader::AUDIO)) {
  // do something about audio
}

Comment 3

[u480271]

Attachment: Bug 1276570: Replace TargetQueues enum with bitwise-or flags.

Review commit: https://reviewboard.mozilla.org/r/56800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56800/

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8758552 [details]
Bug 1276570: Replace TargetQueues enum with bitwise-or flags.

https://reviewboard.mozilla.org/r/56800/#review53518

::: dom/media/MediaDecoderReader.cpp:135
(Diff revision 1)
>  size_t MediaDecoderReader::SizeOfAudioQueueInFrames()
>  {
>    return mAudioQueue.GetSize();
>  }
>  
> -nsresult MediaDecoderReader::ResetDecode(TargetQueues aQueues /*= AUDIO_VIDEO*/)
> +nsresult MediaDecoderReader::ResetDecode(uint32_t aQueues)

Why changing the type from TargetQueues to uint32_t?

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Please use EnumSet to provide type safety.

Comment 6

[u480271]

Comment on attachment 8758552 [details]
Bug 1276570: Replace TargetQueues enum with bitwise-or flags.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56800/diff/1-2/

Comment 7

[u480271]

https://reviewboard.mozilla.org/r/56800/#review53518

> Why changing the type from TargetQueues to uint32_t?

Because you can't assign TargetQueues::AUDIO | TargetQueues::VIDEO to TargetQueues.

Comment 8

[u480271]

(In reply to JW Wang [:jwwang] from comment #5)
> Please use EnumSet to provide type safety.

JW, I switched to using EnumSet. I spoke with jya and decided to change the name too.

Instead of introducing yet-another-enum for specifying audio or video, I've made an EnumSet from TrackInfo::TrackType. Consequently, I've changed TargetQueues to be TrackSet, representing the set of tracks to be reset.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8758552 [details]
Bug 1276570: Replace TargetQueues enum with bitwise-or flags.

https://reviewboard.mozilla.org/r/56800/#review54272

::: dom/media/MediaDecoderStateMachine.cpp:2392
(Diff revision 2)
> -  mVideoCompleted = false;
> +    mVideoCompleted = false;
> -  VideoQueue().Reset();
> +    VideoQueue().Reset();
> +  }
>  
> -  if (aQueues == MediaDecoderReader::AUDIO_VIDEO) {
> +  if (aTracks.contains(TrackInfo::kAudioTrack)) {
>      // Stop the audio thread. Otherwise, MediaSink might be accessing AudioQueue

Assert aTracks.contains(TrackInfo::kVideoTrack) in this if statement because we don't support reset audio only for now.

Comment 10

[u480271]

Comment on attachment 8758552 [details]
Bug 1276570: Replace TargetQueues enum with bitwise-or flags.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56800/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e6e28c8197
Replace TargetQueues enum with bitwise-or flags. r=jwwang

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09e6e28c8197