Closed Bug 988291 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla33
blocking-b2g 1.4+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: seinlin, Assigned: schiu)

References

Details

(Keywords: smoketest, Whiteboard: [Dolphin_1.4])

Attachments

(4 files)

There is no thumbnails for video files recorded by camera.
Blocks: dolphin
(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?
QA Contact: ying.xu
Summary: [dolphin][Thumbnails] No thumbnails for video files recorded by camera → [dolphin][Thumbnails] Thumbnails parses fails for video files decoded by omx codec
Assignee: nobody → ying.xu
QA Contact: ying.xu
Whiteboard: [POVB]
It caused by qcom hard code, not spreadtrum POVB bug.
blocking-b2g: --- → 1.4?
Whiteboard: [POVB]
Depends on: 931733
Whiteboard: [Dolphin_1.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 <ying.xu@spreadtrum.com>
Date:   Wed Mar 19 17:11:19 2014 +0800

    Bug#289335 do not return buffers for display on FFOS system
    
    Change-Id: I3d0c3dd3cab484a51d445bb800e7cb858a727bb
Flags: needinfo?(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 <ying.xu@spreadtrum.com>
> 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.
Flags: needinfo?(brsun)
Please provide us with:

1) STR
2) Is this Dolphin specific
3) User impact if not fixed.
Flags: needinfo?(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.
Depends on: 1005852
Flags: needinfo?(kli)
Ivan

Please chime in.

What RIL is in question here?

Next steps to move ahead are unclear at this point.
Flags: needinfo?(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.
Flags: needinfo?(itsay)
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?
Flags: needinfo?(tchung)
Flags: needinfo?(styang)
(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.
Flags: needinfo?(tchung)
(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?
Flags: needinfo?(styang) → needinfo?(james.zhang)
Steven, Before bug 931733 landed, I think we need to patch gecko like this. This is what we've done for 6821.
Flags: needinfo?(james.zhang)
But we still need to confirm if the patch from 6821 can use on 7715/7730.
I verified that thumbnails can be generated correctly with the patch in comment 13. So 7715/7730 still need this patch for gecko.
Moving it to gfx based on patch in comment 13
Component: Gaia::Video → Graphics: Layers
Product: Firefox OS → Core
Triage: this one is needed in v1.4 for Thumbnail works correctly. Assign to Vincent Liu for further verification on the patch.
Assignee: ying.xu → vliu
blocking-b2g: 1.4? → 1.4+
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.
blocking-b2g: 1.4+ → ---
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
Flags: needinfo?(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.
Flags: needinfo?(mwu)
(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?
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?
Attachment #8438199 - Flags: feedback?(schiu)
This is another possible temporary solution, but too many compile flags do not look good.
Attachment #8438199 - Flags: feedback?(sotaro.ikeda.g)
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
Attachment #8438199 - Flags: feedback?(sotaro.ikeda.g)
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
Attachment #8438199 - Flags: feedback?(schiu) → feedback+
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.
It seems Solomon is working on this. Switching the owner to him.
Assignee: vliu → schiu
Duplicate of this bug: 1027517
(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.
blocking-b2g: --- → 1.4+
Hi, Solomon, may we know that if you're currently working on this? Thank you.
Flags: needinfo?(schiu)
I am working on bug#931733, it should fix this bug too.
Flags: needinfo?(schiu)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
You need to log in before you can comment on or make changes to this bug.