988291 - [dolphin][Thumbnails] Thumbnails parses fails for video files decoded by omx codec

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

There is no thumbnails for video files recorded by camera.

Comment 1

[ying.xu]

this is caused by 
https://bugzilla.mozilla.org/show_bug.cgi?id=931733

Comment 2

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to ying.xu from comment #1)
> this is caused by 
> https://bugzilla.mozilla.org/show_bug.cgi?id=931733

Thanks Ying, can you take this?

Comment 3

[James Zhang (Spreadtrum)]

It caused by qcom hard code, not spreadtrum POVB bug.

Comment 4

[Kai-Zhen Li [:seinlin][:kli]]

Bruce, Could you help me check if the commit is for the bug? If yes, let's close this bug. Thanks!

commit 6a8febafc7a77888a8703980e39d9e398343aea4
Author: ying.xu <[e-mail]>
Date:   Wed Mar 19 17:11:19 2014 +0800

    Bug#289335 do not return buffers for display on FFOS system
    
    Change-Id: I3d0c3dd3cab484a51d445bb800e7cb858a727bb

Comment 5

[Bruce Sun [:brsun]]

(In reply to Kai-Zhen Li from comment #4)
> Bruce, Could you help me check if the commit is for the bug? If yes, let's
> close this bug. Thanks!
> 
> commit 6a8febafc7a77888a8703980e39d9e398343aea4
> Author: ying.xu <[e-mail]>
> Date:   Wed Mar 19 17:11:19 2014 +0800
> 
>     Bug#289335 do not return buffers for display on FFOS system
>     
>     Change-Id: I3d0c3dd3cab484a51d445bb800e7cb858a727bb

That change seems to resolve the same problem as bug 864230. And that is not related to the thumbnail problem caused by unsupported color formats.

Comment 6

[Preeti Raghunath(:Preeti)]

Please provide us with:

1) STR
2) Is this Dolphin specific
3) User impact if not fixed.

Comment 7

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Preeti Raghunath(:Preeti) from comment #6)
> Please provide us with:
> 
> 1) STR
After video is recorded, there is no Thumbnails for it.

> 2) Is this Dolphin specific
I think it is not only Dolphin specific, it could be a Vendor specific bug, as bug 931733.
Before bug 931733 got fixed, we need to handle this issue per device/vendor.

> 3) User impact if not fixed.
Without Thumbnails it is not good to check recored video files. User will see a white box, need to play it to see what it is.


Now there is some problem with video recording before bug 1005852 got fixed.

Comment 8

[Preeti Raghunath(:Preeti)]

Ivan

Please chime in.

What RIL is in question here?

Next steps to move ahead are unclear at this point.

Comment 9

[Ivan Tsay (:ITsay)]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Ivan
> 
> Please chime in.
> 
> What RIL is in question here?
> 
> Next steps to move ahead are unclear at this point.

We need this in v1.4 since our code should not be bound with specific vendor. So I think the dependent bug 931733 should also be nominated to v1.4.

Comment 10

[Hema Koka [:hema]]

Not generating thumbnails for videos is a critical issue for dolphin. Dependency needs to be prioritized.

steven/tony: are we blocking on dolphin bugs for 1.4?

Comment 11

[Tony Chung [:tchung]]

(In reply to Hema Koka [:hema] from comment #10)
> Not generating thumbnails for videos is a critical issue for dolphin.
> Dependency needs to be prioritized.
> 
> steven/tony: are we blocking on dolphin bugs for 1.4?

see https://bugzilla.mozilla.org/show_bug.cgi?id=931733#c17.   this bug seems to be dependent on that bug, which has been backlogged.

in general, i believe the general approach for dolphin 1.4 is to have someone check on the Flame/Open C 1.4 first to see if it reproduces.   if a bug is specific to 1.4 and Kitkat configuration (which dolphin is), we can take those bugs as case by case issue.

Comment 12

[Steven Yang [:styang]]

(In reply to James Zhang from comment #3)
> It caused by qcom hard code, not spreadtrum POVB bug.

what do you mean by hardcoded? can you specify the codes?

Comment 13

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: gecko-GrallocImages-HAL_PIXEL_FORMAT.patch

Steven, Before bug 931733 landed, I think we need to patch gecko like this. This is what we've done for 6821.

Comment 14

[Kai-Zhen Li [:seinlin][:kli]]

But we still need to confirm if the patch from 6821 can use on 7715/7730.

Comment 15

[Kai-Zhen Li [:seinlin][:kli]]

I verified that thumbnails can be generated correctly with the patch in comment 13. So 7715/7730 still need this patch for gecko.

Comment 16

[Hema Koka [:hema]]

Moving it to gfx based on patch in comment 13

Comment 17

[Ivan Tsay (:ITsay)]

Triage: this one is needed in v1.4 for Thumbnail works correctly. Assign to Vincent Liu for further verification on the patch.

Comment 18

[Ivan Tsay (:ITsay)]

We should not do another hardcode as the temp solution. De-nominate this bug for now and we shall keep following the issue on bug 931733 for the partner independent solution on HAL_PIXEL_FORMAT.

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: dolphin_patches.diff

Michael, 

For dolphin, we need a local patch for pixel format before bug 931733 get fixed. After discussed with Releng, if the patch can repo sync from manifest could be better for pvt build.

So I'd like to add a 'patches' into dolphin manifest, as attached diff.

Before this I think we should confirm where the patches should be maintained. Now the patches is maintained on my github.
https://github.com/Seinlin/dolphin-patches

If you also think add a patches for dolphin is reasonable, 
could you help me init a repo of 'dolphin-patches' on mozilla-b2g?

And then I'll send PR to 
- dolphin-patches and
- dolphin manifest

Comment 20

[Michael Wu [:mwu]]

r- on patching gecko, by policy. The patch directory was originally provided to patch gonk pieces, not gecko. It is never acceptable to use the patch directory to patch gecko.

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Wu [:mwu] from comment #20)
> r- on patching gecko, by policy. The patch directory was originally provided
> to patch gonk pieces, not gecko. It is never acceptable to use the patch
> directory to patch gecko.

Thanks! Not allowed to patch gecko seems reasonable. So we still need to find another possible solution for it. Do you have any suggestion?

Comment 22

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: pixel-format.patch

Sotaro and Solomon, could you have a look to this wip? Is it possible to land this temporary solution before we have a better solution?

Comment 23

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: pixel-format_2.patch

This is another possible temporary solution, but too many compile flags do not look good.

Comment 24

[Sotaro Ikeda [:sotaro]]

It seems to unavoidable to add vendor specific conifig/ifdef. One similar example is "TARGET_USES_ION". It is added to BoardConfig.mk by Bug 1009297. That config is referenced by /qcom/media/mm-video-v4l2/vidc/vdec/Android.mk. It adds USE_ION definition and is used in cpp files.

> ifeq ($(TARGET_USES_ION),true)
> libOmxVdec-def += -DUSE_ION
> endif

It think the following is simple solution to this problem.
- Add a vendor specific ifdef for color format values
- change color format depends on the vendor specific ifdef

Comment 25

[Solomon Chiu [:schiu]]

Comment on attachment 8438199 [details] [diff] [review]
pixel-format.patch

I agree with Sotaro's comment. And I also created some patches in the blocking bug - #931733 to gather comments from gfx team. If we want to use ifdef to do conditional compilation, we maybe need to distinguish those format base on GPU platform. So I use "export GPU_PLATFORM := mali" in BoardConfig.mk to propagate the definition to GrallocImage.*. The remaining problem is - the pre-processor cannot handle string comparing, so we might have to tweak the definition further as we discussed yesterday, such as:

GPU_USE_ADRENO := 1
GPU_USE_MALI := 2
GPU_USE_POWERVR := 3

export GPU_PLATFROM := GPU_USE_MALI

And in GrallocImages.h we also some definition like:

#define GPU_USE_ADRENO 1
#define GPU_USE_MALI   2
#define GPU_USE_POWERVR 3
    :
#if GPU_PLATFORM == GPU_USE_MALI
enum {
    // vendor extended format here
}
#end

Comment 26

[Ivan Tsay (:ITsay)]

To proceed for creating the Dolphin pvtbuild, this bug will be listed as the known issue in Dolphin pvtbuild. The proper solution will be provided in 2.0 by fixing the bug 846421 and bug 880114.

Comment 27

[Vincent Liu[:vliu]]

It seems Solomon is working on this. Switching the owner to him.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Ivan Tsay (:ITsay) from comment #18)
> We should not do another hardcode as the temp solution. De-nominate this bug
> for now and we shall keep following the issue on bug 931733 for the partner
> independent solution on HAL_PIXEL_FORMAT.

This is tracking the end user behavior, which is currently a smoketest blocker. If bug 931733 fixes this, then feel free to close this out after that lands. But blocking this because it's a smoketest blocker.

Comment 30

[Kai-Chih Hu [:khu]]

Hi, Solomon, may we know that if you're currently working on this? Thank you.

Comment 31

[Solomon Chiu [:schiu]]

I am working on bug#931733, it should fix this bug too.

Comment 32

[Seth Miko [:SMiko]]

Verified fixed on Flame 2.0 (319mb/shallow flash),and Flame 2.1 (319mb/shallow flash)

Actual result: Recorded videos have thumbnails.

Device: Flame 2.0
BuildID: 20141110000204
Gaia: d3e4da377ee448f9c25f908159480e867dfb13f3
Gecko: 7198906837e7
Gonk: 
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Gonk: Could not pull gonk.  Did you shallow Flash?
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0