Closed Bug 1033910 Opened 10 years ago Closed 10 years ago

Enable RTSP capability while using android::MediaCodec.

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: brsun, Assigned: bechen)

References

Details

Attachments

(1 file, 3 obsolete files)

This is a followup bug of bug 904177. RTSP capability has not been integrated in MediaCodecReader. This capability is needed to be integrated into MediaCodecReader.
Blocks: 1033935
Blocks: 1033969
Assignee: nobody → bechen
feature-b2g: --- → 2.1
Target Milestone: --- → 2.1 S3 (29aug)
Attached patch WIP-bug-1033910.v01.patch (obsolete) — Splinter Review
Attached patch bug-1033910.v01.patch (obsolete) — Splinter Review
1. Extract two classes RtspExtractor and RtspMediaSource from RtspOmxReader.cpp to RtspExtractor.cpp. Because the RtspOmxReader and the RtspMediaCodecReader both need these classes.

2. Move |MediaCodecReader::CreateExtractor()| and |MediaCodecReader::mExtractor|
 to protected member becuase the RtspMediaCoderReader need to provide a new extractor.

3. RtspMediaCodecReader: Creates a RtspExtractor for rtsp streaming.
There is a |seek(0)| in RtspMediaCodecReader::ReadMetadata to deal with bug 105664.
Attachment #8474477 - Attachment is obsolete: true
Attachment #8477336 - Flags: feedback?(slee)
Attachment #8477336 - Flags: feedback?(brsun)
Comment on attachment 8477336 [details] [diff] [review]
bug-1033910.v01.patch

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

lgtm. :)

::: content/media/omx/RtspMediaCodecReader.cpp
@@ +45,5 @@
> +  // The seek function of Rtsp is time-based, we call the SeekTime function in
> +  // RtspMediaResource. The SeekTime function finally send a seek command to
> +  // Rtsp stream server through network and also clear the buffer data in
> +  // RtspMediaResource.
> +  if (mRtspResource) {

mRtspResource is assigned in constructor. There is no other places to change it. If you are worried about mRtspResource could be null, I think it's better to use assertion, for here and the below.

::: content/media/omx/RtspMediaCodecReader.h
@@ +58,5 @@
> +  // Disptach a DecodeVideoFrameTask to decode video data.
> +  virtual void RequestVideoData(bool aSkipToNextKeyframe,
> +                                int64_t aTimeThreshold) MOZ_OVERRIDE;
> +
> +  // Disptach a DecodeAduioDataTask to decode video data.

audio data
Attachment #8477336 - Flags: feedback?(slee) → feedback+
Attached patch bug-1033910.v01.patch (obsolete) — Splinter Review
Chris:
Need your help to review this because Bruce is not availible for these days, he got a newborn baby :). Detail in comment 2.
Attachment #8477336 - Attachment is obsolete: true
Attachment #8477336 - Flags: feedback?(brsun)
Attachment #8478155 - Flags: review?(cpearce)
(In reply to Benjamin Chen [:bechen] from comment #2)
> Created attachment 8477336 [details] [diff] [review]
> bug-1033910.v01.patch
> 
> 1. Extract two classes RtspExtractor and RtspMediaSource from
> RtspOmxReader.cpp to RtspExtractor.cpp. Because the RtspOmxReader and the
> RtspMediaCodecReader both need these classes.
> 
> 2. Move |MediaCodecReader::CreateExtractor()| and
> |MediaCodecReader::mExtractor|
>  to protected member becuase the RtspMediaCoderReader need to provide a new
> extractor.
> 
> 3. RtspMediaCodecReader: Creates a RtspExtractor for rtsp streaming.
> There is a |seek(0)| in RtspMediaCodecReader::ReadMetadata to deal with bug
> 105664.

Typo, it's bug 1050664.
Blocks: 1058429
Comment on attachment 8478155 [details] [diff] [review]
bug-1033910.v01.patch

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

::: content/media/omx/MediaOmxCommonDecoder.h
@@ +21,5 @@
>  class MediaOmxCommonDecoder : public MediaDecoder
>  {
>  public:
>    MediaOmxCommonDecoder();
> +  virtual ~MediaOmxCommonDecoder();

MediaDecoder is refcounted, so the destructor should be private or protected.

::: content/media/omx/RtspMediaCodecDecoder.cpp
@@ +39,5 @@
> +  MediaDecoder::ApplyStateToStateMachine(aState);
> +
> +  // Notify RTSP controller if the play state is ended.
> +  // This is necessary for RTSP controller to transit its own state.
> +  if (aState == PLAY_STATE_ENDED) {

I know this was in the old code, but the only time we set MediaDecoder to PLAY_STATE_ENDED is when we seek to the end of stream, or if playback reaches there naturally.

If we seek to the end of stream, the MediaDecoderReader should know it's reached end stream right?

If playback reaches there naturally, the MediaDecoderReader has to mark its samples as end-of-stream, so the reader must know it's end of stream because it called MediaDecoderReaderCallback::On{Audio,Video}EOS(), or it returned false from MediaDecoderReader::DecodeAudioData() or MediaDecoderReader::DecodeVideoFrame().

So it seems to me that you could avoid overriding this function. Fixing this in a follow up bug is fine with me, since you're just moving existing working code around here.

::: content/media/omx/RtspMediaCodecDecoder.h
@@ +19,5 @@
> +  virtual MediaOmxCommonReader* CreateReader() MOZ_OVERRIDE;
> +
> +  virtual MediaDecoderStateMachine* CreateStateMachine(MediaOmxCommonReader* aReader) MOZ_OVERRIDE;
> +
> +  virtual void ApplyStateToStateMachine(PlayState aState)MOZ_OVERRIDE;

s/ApplyStateToStateMachine(PlayState aState)MOZ_OVERRIDE/ ApplyStateToStateMachine(PlayState aState) MOZ_OVERRIDE/

::: content/media/omx/RtspMediaCodecReader.cpp
@@ +101,5 @@
> +                                   MetadataTags** aTags)
> +{
> +  // TODO: fix me, we need a SeekTime(0) here because the
> +  // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
> +  mRtspResource->SeekTime(0);

Perhaps you should only seek if ReadMetadata() returned NS_OK, and the MediaCodecReader::IsWaitingMediaResources() is false?

I'm not sure if that will work, but I think you should figure out if it does, as it will save resources if you do.

::: content/media/omx/RtspMediaCodecReader.h
@@ +58,5 @@
> +  // Disptach a DecodeVideoFrameTask to decode video data.
> +  virtual void RequestVideoData(bool aSkipToNextKeyframe,
> +                                int64_t aTimeThreshold) MOZ_OVERRIDE;
> +
> +  // Disptach a DecodeAduioDataTask to decode audio data.

s/DecodeAduioDataTask/DecodeAudioDataTask/

(spelling mistake)
Attachment #8478155 - Flags: review?(cpearce) → review+
r=cpearce
Push to try server again. 
https://tbpl.mozilla.org/?tree=Try&rev=60e6757c5740

After discussed with Bruce, we modify the ReadMetadata fuction and adjust the SeekTime(0) position to make sure the mStartTime in StateMachine is correct.
Attachment #8478155 - Attachment is obsolete: true
Attachment #8480485 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e8db8f5d142
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: