Closed Bug 925381 Opened 6 years ago Closed 6 years ago

Fail to seek mpeg4 files in video player

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.3 Sprint 3 - 10/25
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: bhargavg1, Assigned: sotaro)

References

Details

(Keywords: regression, Whiteboard: [PRISM:552721])

Attachments

(2 files, 4 obsolete files)

Attached video VID_20131010_091241.3gp
On playing mpeg4 files in video app, seeking fwd by scrubbing the bar doesn't play the video only audio is heard
Blocks: 916996
blocking-b2g: --- → koi?
Whiteboard: [PRISM:552721]
Can you provide hardware info and software version?
Hardware: MSM7x27a QRD device
BuildId: 20131009193215
(In reply to bhargavg1 from comment #2)
> Hardware: MSM7x27a QRD device
> BuildId: 20131009193215

can you provide git info of gecko and gaia? The hardware seems like not Firefox OS phone. Does the soft uses caf's android ICS code? And can you provide a content that generate the problem?
Flags: needinfo?(bhargavg1)
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> (In reply to bhargavg1 from comment #2)
> > Hardware: MSM7x27a QRD device
> > BuildId: 20131009193215
> 
> can you provide git info of gecko and gaia? The hardware seems like not
> Firefox OS phone. Does the soft uses caf's android ICS code? And can you
> provide a content that generate the problem?


SHA1's
Gecko: c038ee40141d22e088e4777355d2be0c3e673aed
Gaia:  672c47bf94b69a329e0aacb9228a6aa16ade6226
Flags: needinfo?(bhargavg1)
Assignee: nobody → sotaro.ikeda.g
Thanks for the infos and video. Confirmed the problem on my master hamachi. It is a regression.
gralloc buffers seems not returned to ANativeWindow correctly during seeking.
Confirmed that the problem also happens by attachment 742194 [details] in Bug 864230.
QA Contact: nkot
I quickly tested with several .mp4 and .3gpp videos on keon with a recent-ish (~1 week old) gecko+gaia, and could not reproduce the issue.

Could this be a bad interaction between how video playback work and the device running out of pmem?
the first time the issue was noticed in the following CAF manifest AU
https://www.codeaurora.org/cgit/quic/lf/manifest/tree/caf_AU_LINUX_GECKO_B2G_ICS_1.2.01.02.00.019.057.xml?id=refs/heads/release

Gecko: 52eeb289cc638408fff6be89835e2c1e121bd955
Gaia:  b0e4a1333bb7bf0a749a384ba99e4f03f111e39a
(In reply to Nicolas Silva [:nical] from comment #10)
> I quickly tested with several .mp4 and .3gpp videos on keon with a
> recent-ish (~1 week old) gecko+gaia, and could not reproduce the issue.
> 
> Could this be a bad interaction between how video playback work and the
> device running out of pmem?

I could reproduce it with the video attached in comment 9, 
I am rying to find a regression range, so far tested on the latest m-c and 09/18 m-c - the issue reproduces on both
How do we know that this is a regression issue? Is it from 1.1 or within the 1.2 cycle?
Flags: needinfo?(nkot)
It is a regression from 1.1. on v1.2, video playback does not work correctly until Bug 907745 was fixed. If the mpeg4 seek has a problem all time since Bug 907745 was fixed, there is no regression range in v1.2.
I understand the reason of the regression from v1.1. Clear 'regressionwindow-wanted'.
As in Bug 864230, mpeg4 hw codec requires more buffer than h.264. b2g v1.1 achieved the smooth mpeg4 playback. But it was not easy. During mpeg4 playback, buffers usage is very very tight. And in v1.1, composer holds 1 buffer at most. But in v1.2, composer holds 2 buffer. During seeking, it makes buffer starvation on hamachi and leo devices.
on v1.1, composer holds 1 buffer. But on v1.2, composer holds 2 buffers. I locally created a v1.2 ROM that composer does not hold a buffer during seeking. By the ROM, v1.2 hamachi succeeds mpeg4 seek. But it causes black screen during seeking. It is not acceptable from user experience point of view. To fix it, need add a way to hold only 1 buffer during seek.
android's BufferQueue already implements similar functions BufferQueue::freeAllBuffersExceptHeadLocked(). gecko also needs to implement similar thing.

  http://androidxref.com/4.3_r2.1/xref/frameworks/native/libs/gui/BufferQueue.cpp#988
blocking-b2g: koi? → koi+
Priority: -- → P1
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> It is a regression from 1.1. on v1.2, video playback does not work correctly
> until Bug 907745 was fixed. If the mpeg4 seek has a problem all time since
> Bug 907745 was fixed, there is no regression range in v1.2.

Thanks Sotaro!
yes, the issue doesn't reproduce on v1.1, I've tested that too. Please let me know if you need any help determining regression within 1.2 cycle. From my testing, i could consistently reproduce this bug starting 09/13, but it's possible that it occurred earlier, it's just really hard to say due to other playback issue on earlier builds.

BuildID: 20130913040201
Gaia: 8ccb741b6adcfe9a78b842c17e5874242c0f8b86
Gecko: b9029b1de410
Version: 26.0a1
Flags: needinfo?(nkot)
(In reply to nkot from comment #19)
> (In reply to Sotaro Ikeda [:sotaro] from comment #14)
> yes, the issue doesn't reproduce on v1.1, I've tested that too. Please let
> me know if you need any help determining regression within 1.2 cycle. From
> my testing, i could consistently reproduce this bug starting 09/13, but it's
> possible that it occurred earlier,

On v1.2, video playback was basically broken until Bug 901224 and Bug 907745 were fixed. Because of the broken playback, seek did not cause the buffer starvation until 09/13 on v1.2, I think.
Thanks for the confirmation.
By applying the patch, mpeg4 seek works on master hamachi.
(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> Created attachment 816050 [details] [diff] [review]
> patch - return all video frames except front one during seek
> 
> By applying the patch, mpeg4 seek works on master hamachi.

Can you let me know the based build id that you are using. Facing issues in applying the patch
(In reply to bhargavg1 from comment #23)
> Can you let me know the based build id that you are using. Facing issues in
> applying the patch

I use master gecko, git commit id is following.
gecko: ad67fba436dbdb4b8f66be7e9b1783bd41bdcd90
Yes works for me too !
(In reply to bhargavg1 from comment #25)
> Yes works for me too !

Thanks for the confirmation.
(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> (In reply to bhargavg1 from comment #25)
> > Yes works for me too !
> 
> Thanks for the confirmation.

Can we merge this change?
Flags: needinfo?(sotaro.ikeda.g)
Target Milestone: --- → 1.2 C3(Oct25)
(In reply to bhargavg1 from comment #27)
> > Thanks for the confirmation.
> 
> Can we merge this change?

Yes, I am going to update the patch.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #816050 - Attachment is obsolete: true
Attachment #817435 - Attachment is obsolete: true
Attachment #817440 - Flags: review?(nical.bugzilla)
Attachment #817441 - Flags: review?(chris.double)
Attachment #817441 - Flags: review?(chris.double) → review+
Comment on attachment 817440 [details] [diff] [review]
patch part 1 - Change to /gfx/layers/

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

LGTM. I just have the following remark that we are at the same time trying to enforce constraints on the life cycles of textures (like in this bug) while at the same time trying to reduce them in order to be able to share them to several layers at the same time (Bug 912897). I don't think those two approaches will play well together in the long run. We'll figure that out later, but we should be careful when writing code that expects TextureClients to strictly belong to one CompositableClient (or ImageClient in this case), because we are trying to move away from that.

::: gfx/layers/ImageContainer.cpp
@@ +190,1 @@
>      // Let ImageClient to release all TextureClients.

nit: you can also remove the "to" to fix this comment since you are modifying the surrounding code.

@@ +202,5 @@
> + void
> +ImageContainer::ClearAllImages()
> +{
> +  if (IsAsync()) {
> +    // Let ImageClient to release all TextureClients.

nit: same here, let's remove the "to"

::: gfx/layers/ImageContainer.h
@@ +390,5 @@
> +  void ClearAllImages();
> +
> +  /**
> +   * Clear all images except current one. 
> +   * Let ImageClient to release all TextureClients except front one. 

nit: trailing spaces
Attachment #817440 - Flags: review?(nical.bugzilla) → review+
Component: Gaia::Video → General
Roll up patch. Apply comments. Carry 'nical.bugzilla: review+' and chris.double: review+.
Attachment #817440 - Attachment is obsolete: true
Attachment #817441 - Attachment is obsolete: true
Attachment #818029 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b73d5834399
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 1.2 C3(Oct25) → 1.3 Sprint 3 - 10/25
You need to log in before you can comment on or make changes to this bug.