812756 - Segfault while playing MP4 & hanging for playing second MP4

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Thinker Li [:sinker]]

Video app got segfault while playing MP4, and hanging while playing MP4 if previous playing is success.

The cause of segfault is OMXCodec does not allow returning buffers while flushing.  It should be a bug of stagefright.  OMXCodec::signalBufferReturned() does not allow any returning while output port is SHUTTING_DOWN.  But, output port is also in SHUTTING_DOWN while flushing.  OMXCodec should remember buffers while flushing, and handle them while flushing is done.

The cause of hanging while playing second MP4 is timed-out error of first playing.  It is also a bug of stagefright.  mState of OMXCodec should be ERROR while flushing is timed-out, but it stays in FLUSHING state.  It make OMXCodec::stop() goes to a infinite waiting.

Comment 1

[Thinker Li [:sinker]]

Attachment: Fix OMX buffer returning and timed-out issue

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 682736 [details] [diff] [review]
Fix OMX buffer returning and timed-out issue

I don't think this is really a bug in OMXCodec. We appear to be returning buffers when it doesn't expect it.

I added a hack a few months back to our catchup code to seek to the next keyframe if playback is falling behind. What I _think_ is going on here (as far as the segfault goes) is:

1. The video falls behind
2. We try to seek forward to the next keyframe, causing the OMXCodec to transition into the FLUSHING state
3. One of the frames sitting on the VideoQueue is rendered, releasing the old frame
4. The old frame is released, calling signalBufferReturned
5. The assertion fails, and we crash

I suspect that if we clear the VideoQueue _before_ seeking, there will be no frames to be released _while_ seeking.

I can't repro reliably, but I'll put up a patch to fix it soon.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Flush VideoQueue before seeking/catching up

This patch seems to stop the segfault while playing for me. Can you try it?

Freezing on playing a second video seems to have fixed itself with a gaia update.

Comment 4

[Thinker Li [:sinker]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #2)
> Comment on attachment 682736 [details] [diff] [review]
> Fix OMX buffer returning and timed-out issue
> 
> I don't think this is really a bug in OMXCodec. We appear to be returning
> buffers when it doesn't expect it.
> 
> I added a hack a few months back to our catchup code to seek to the next
> keyframe if playback is falling behind. What I _think_ is going on here (as
> far as the segfault goes) is:
> 
> 1. The video falls behind
> 2. We try to seek forward to the next keyframe, causing the OMXCodec to
> transition into the FLUSHING state
> 3. One of the frames sitting on the VideoQueue is rendered, releasing the
> old frame
> 4. The old frame is released, calling signalBufferReturned
> 5. The assertion fails, and we crash
> 
> I suspect that if we clear the VideoQueue _before_ seeking, there will be no
> frames to be released _while_ seeking.
> 
> I can't repro reliably, but I'll put up a patch to fix it soon.
Actually, I had a patch to fix it at first moment by queuing all buffer returning while seeking.  It works perfect for me.  But, after some studying of OMXCodec's history, I think it is a normal behavior of OMXCodec.  In the elder visions of OMXCodec, there is not FLUSHING state, users of OMXCodec can return buffer at any instant of executing.  But, after introduce FLUSHING state, I guess they forget to handle necessary changes at signalBufferReturned().

It makes no sense to forbid buffer returning while flushing since these buffers are supposed to be used later.  OMXCodec should keep them in a free list, or mark them free (OWNED_BY_US), and reuse them when need.  If we are not likely to change OMXCodec.cpp, we should queuing the requests on our own.

Comment 5

[Thinker Li [:sinker]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #3)
> Created attachment 683363 [details] [diff] [review]
> Flush VideoQueue before seeking/catching up
> 
> This patch seems to stop the segfault while playing for me. Can you try it?
> 
> Freezing on playing a second video seems to have fixed itself with a gaia
> update.

I had tried the same change as suggestion from another bug.  But, it is still buggy.  signalBufferReturned() is called async.  You never know when the VideoFrameContainer releases the buffer hold.  So, it is still segfault occasionally.  You can enforce VideoFrameContainer to release it, but ImageContainer still hold a reference to the buffer.

Comment 6

[Thinker Li [:sinker]]

Attachment: Queue buffer returning requests while seeking/flushing

This is the patch I had mentioned above.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Hmm yeah, after testing it out a little bit more it's still broken with my patch, just more intermittent.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

The current image is only suspended when a new image is set. Maybe if we can suspend the VideoFrameContainer for the duration of the read() call from consuming a new image from the video queue, then we will still own everything in the video queue and the ImageContainer will hold on to the buffer that is currently set.

Comment 9

[Thinker Li [:sinker]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> The current image is only suspended when a new image is set. Maybe if we can
> suspend the VideoFrameContainer for the duration of the read() call from
> consuming a new image from the video queue, then we will still own
> everything in the video queue and the ImageContainer will hold on to the
> buffer that is currently set.

I like this solution.  Would you like to make a patch or/and take this bug?

Comment 10

[Andreas Gal :gal]

Edwin, our partners need a patch for this today. Is that possible?

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Should have a patch up for review soon.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Drop frames instead of seeking when falling behind

Comment 13

[cajbir (:cajbir)]

Comment on attachment 685319 [details] [diff] [review]
Drop frames instead of seeking when falling behind

cpearce, is does this match to the skip keyframe change you suggested?

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 685319 [details] [diff] [review]
Drop frames instead of seeking when falling behind

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

Yes.

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76414ce478c3

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Oops, seems to have cleared that field for some reason

Comment 17

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Lower AmpleVideoFrames

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Horrifically ugly hack, this patch, but necessary to avoid a crash on shutdown of the OmxDecoder

Comment 19

[cajbir (:cajbir)]

Comment on attachment 685504 [details] [diff] [review]
Lower AmpleVideoFrames

>+  // Set to the minimum to prevent a race when we finish decoding MPEG4 videos

Can you explain this in more detail please. What is the race and how does lowering the value of ample video frames prevents it?

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76414ce478c3

Comment 21

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Lower AmpleVideoFrames

Comment 22

[cajbir (:cajbir)]

Comment on attachment 685834 [details] [diff] [review]
Lower AmpleVideoFrames

>+  // Due to a bug in the OMX MPEG4 decoder, we can't own too many video buffers

Change "OMX MPEG4 decoder" to the exact name of the decoder (ie. google.what.ever.mpeg4).

>+  // So we need few enough buffers in the queue that all buffers will be
>+  // returned before OMXCodec begins shutdown.
>+  uint32_t GetAmpleVideoFrames() { return 2; }

Sounds like a wonderful source of intermittent bugs coming up related to timing. r+ for the workaround but we should investigate a better way of dealing with this. Can you raise a bug for fixing or investigating a fix for this so it doesn't get forgotten.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6753a211e0a6
https://hg.mozilla.org/releases/mozilla-beta/rev/838c9aa243ad

Comment 24

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db3b8d8ed8f

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0db3b8d8ed8f

Comment 26

[Inder]

Edwin: your patch in comment 24 doesn't apply on beta branch. There are significant differences in files so couldn't apply manually. Can you please rebase your patch for beta branch and share. I need to verify that mp4 files are working fine after your changes.

Comment 27

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Lower AmpleVideoFrames (rebased against beta)

Comment 28

[cajbir (:cajbir)]

Comment on attachment 682736 [details] [diff] [review]
Fix OMX buffer returning and timed-out issue

Edwin has already addressed this so clearing flag.

Comment 29

[Inder]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #27)
> Created attachment 686239 [details] [diff] [review]
> Lower AmpleVideoFrames (rebased against beta)
Edwin: can you please get this patch landed on beta asap.

Comment 30

[JP Rosevear [:jpr]]

New Zealand office is all out on Nov 30th - can someone else land this patch?

Comment 31

[Gregor Wagner [:gwagner]]

(In reply to JP Rosevear [:jpr] from comment #30)
> New Zealand office is all out on Nov 30th - can someone else land this patch?

https://hg.mozilla.org/releases/mozilla-beta/rev/79aae58dfb76